You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/06/11 17:41:59 UTC
[GitHub] [zookeeper] jhuan31 opened a new pull request #1379: ZOOKEEPER-3859: Add a couple request processor metrics
jhuan31 opened a new pull request #1379:
URL: https://github.com/apache/zookeeper/pull/1379
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1379: ZOOKEEPER-3859: Add a couple request processor metrics
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1379:
URL: https://github.com/apache/zookeeper/pull/1379#discussion_r444624406
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
##########
@@ -118,6 +120,8 @@ public void run() {
break;
}
}
+ } catch (RuntimeException e) { // spotbugs require explicit catch of RuntimeException
Review comment:
I guess I just don't understand why "spotbugs require explicit catch of RuntimeException" . We already have this `catch (Exception e)`, which also catch `RuntimeException`. No need to repeat the code that does same thing twice. Especially in our case, the intention was to deal with all exceptions in `handleException `, which is against what spotbug recommanded as "to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions", so no need to syntactically please spot bug but semantically against its recommendation.
In any case, if such change is a hard requirement to get a green build, I am ok with it. Just don't understand why spotbug was complaining this way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli closed pull request #1379: ZOOKEEPER-3859: Add a couple request processor metrics
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #1379:
URL: https://github.com/apache/zookeeper/pull/1379
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] jhuan31 commented on a change in pull request #1379: ZOOKEEPER-3859: Add a couple request processor metrics
Posted by GitBox <gi...@apache.org>.
jhuan31 commented on a change in pull request #1379:
URL: https://github.com/apache/zookeeper/pull/1379#discussion_r444494946
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
##########
@@ -118,6 +120,8 @@ public void run() {
break;
}
}
+ } catch (RuntimeException e) { // spotbugs require explicit catch of RuntimeException
Review comment:
The RuntimeException is not exactly "swallowed" here. It is handled as if it were a non-RuntimeException and the zookeeper server is stopped--is it the right thing to do?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1379: ZOOKEEPER-3859: Add a couple request processor metrics
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1379:
URL: https://github.com/apache/zookeeper/pull/1379#discussion_r442518337
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
##########
@@ -118,6 +120,8 @@ public void run() {
break;
}
}
+ } catch (RuntimeException e) { // spotbugs require explicit catch of RuntimeException
Review comment:
We already catch exception [here](https://github.com/apache/zookeeper/pull/1379/files#diff-0502d4c8738b8641bf3fbb72d15066d8L121), which should also catch `RuntimeException`.
What's the exact guideline that spot bug was proposing? I did a quick search, the closest match I could find is:
https://http-builder-ng.github.io/http-builder-ng/okhttp/spotbugs/main.html
Where it said: `This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught. It is a common bug pattern to say try { ... } catch (Exception e) { something } as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs.
A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below:
try {
...
} catch (RuntimeException e) {
throw e;
} catch (Exception e) {
... deal with all non-runtime exceptions ...
}
`
Which sound plausible, but we don't do either (we didn't rethrow the caught runtime exception).
Should we rethrow the RuntimeException caught here? It sounds not usual to just swallow the RuntimeException.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1379: ZOOKEEPER-3859: Add a couple request processor metrics
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1379:
URL: https://github.com/apache/zookeeper/pull/1379#issuecomment-648634988
merging now, thank you @jhuan31
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org