Fix #976 - Expand overly broad Exceptions in Accumulo-core #1018
Conversation
- Expand overly broad Exceptions that are masking the true Exception in accumulo-core - Formatting and Fixing Unit Test
| this.blockCacheManager.stop(); | ||
| } | ||
| } catch (Exception e1) { | ||
| } catch (RuntimeException e1) { |
There was a problem hiding this comment.
This catch block could probably be removed.
There was a problem hiding this comment.
this.blockCacheManager.stop(); has the possibility to throw an UnsupportedOperationException so I can catch/throw that here to narrow it.
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
I'm in favor of this... like a lot... but I'm curious: How/when should we determine that this multi-catch block should also include RTEs and when shouldn't they? (Similar question for "Error" types, when we catch "Throwable", but I think that answer is a little more clear, because generally, catching "Error" should be avoided entirely.)
| } catch (RuntimeException e1) { | ||
| throw new RuntimeException(e1); | ||
| } catch (UnsupportedOperationException e1) { | ||
| throw new UnsupportedOperationException(e1); |
There was a problem hiding this comment.
I'm confused by this. Why not just let e1 get thrown all the way through?
There was a problem hiding this comment.
Or just don't catch it in the first place.
| failedToAcquireLock(); | ||
| } | ||
| } catch (Throwable e) { | ||
| } catch (KeeperException | InterruptedException e) { |
There was a problem hiding this comment.
I don't think this catch should be narrowed because any unexpected runtime exception needs to be properly handled for zookeeper locks. We don't want to ignore an NPE and think the lock is held when it is not.
I worry about the correctness of this change because of possible issues like this. Narrowing could lead to handling runtime exceptions differently which in some case could cause bugs. Its hard to review all of the narrowing for correctness.
There was a problem hiding this comment.
Could just do (KeeperException | InterruptedException | RuntimeException e). I still think there's merit in no longer catching Error and thinking we have any clue how to handle that case (especially with merely logging it).
There was a problem hiding this comment.
This code does more than log, it calls lockWatcher.unableToMonitorLockNode(e) letting an observer know that the state of the lock is now unknown. With ZK lock issues we do want to make the best possible effort to halt the process, even if we fail trying. It may make sense to catch Throwable in case, I don't know without further inspection of the code.
keith-turner
left a comment
keith-turner
left a comment
There was a problem hiding this comment.
Was looking over this again before merging and changed my review. I am uncertain about merging this now because of the comment I made about changing behavior of handling runtime exceptions.
|
@cjmctague I am removing the 2.0 label for this PR. If you want to make these changes for 2.1 that may be more appropriate. |
|
I think it would be great to revisit this, but with special care to avoid any behavior changes at all. It'd be easier to get these changes in if we didn't have to reconsider the kinds of things we are catching and how we are handling things. After that is done, if there are follow-up changes that improve on the behavior changes, I think those would be easier to evaluate on their own. |
|
This PR is quite out-of-date now. This should be rebased onto the main branch, or closed, and re-worked in a new PR. |
4 participants
in accumulo-core