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/12/23 05:52:05 UTC
[GitHub] [zookeeper] lanicc opened a new pull request #1562: ZOOKEEPER-4033: Remove unnecessary judgment of null
lanicc opened a new pull request #1562:
URL: https://github.com/apache/zookeeper/pull/1562
----------------------------------------------------------------
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] breed commented on pull request #1562: ZOOKEEPER-4033: Remove unnecessary judgment of null
Posted by GitBox <gi...@apache.org>.
breed commented on pull request #1562:
URL: https://github.com/apache/zookeeper/pull/1562#issuecomment-750936161
sorry to be late to the discussion, but it is still possible to get a NullPointerException, correct? metricsProvider will be null if an exception is thrown.
----------------------------------------------------------------
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] lanicc commented on a change in pull request #1562: ZOOKEEPER-4033: Remove unnecessary judgment of null
Posted by GitBox <gi...@apache.org>.
lanicc commented on a change in pull request #1562:
URL: https://github.com/apache/zookeeper/pull/1562#discussion_r547877484
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerMain.java
##########
@@ -233,12 +233,10 @@ public void runFromConfig(QuorumPeerConfig config) throws IOException, AdminServ
// warn, but generally this is ok
LOG.warn("Quorum Peer interrupted", e);
} finally {
- if (metricsProvider != null) {
- try {
- metricsProvider.stop();
- } catch (Throwable error) {
- LOG.warn("Error while stopping metrics", error);
- }
+ try {
Review comment:
It's ok
----------------------------------------------------------------
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] ztzg commented on pull request #1562: ZOOKEEPER-4033: Remove unnecessary judgment of null
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1562:
URL: https://github.com/apache/zookeeper/pull/1562#issuecomment-750929863
Merged into `master`; thanks!
----------------------------------------------------------------
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] ztzg commented on pull request #1562: ZOOKEEPER-4033: Remove unnecessary judgment of null
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1562:
URL: https://github.com/apache/zookeeper/pull/1562#issuecomment-750944437
> it is still possible to get a NullPointerException, correct? metricsProvider will be null if an exception is thrown.
I don't think so; unless I have missed something, `MetricsProviderBootstrap.startMetricsProvider` throws if something fails, and while `QuorumPeerMain.runFromConfig` catches the declared exception, it immediately rethrows another. As for unchecked exceptions, they would just bubble up.
(That being said, I agree that this case is much less clear-cut than the typical `var = new Something(...)` before a `try`. It is not obvious that `startMetricsProvider` unless one looks at the code. I have made a mental note to look into these metrics provider mechanisms—and perhaps even document them a bit more!)
----------------------------------------------------------------
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 a change in pull request #1562: ZOOKEEPER-4033: Remove unnecessary judgment of null
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1562:
URL: https://github.com/apache/zookeeper/pull/1562#discussion_r547870163
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerMain.java
##########
@@ -233,12 +233,10 @@ public void runFromConfig(QuorumPeerConfig config) throws IOException, AdminServ
// warn, but generally this is ok
LOG.warn("Quorum Peer interrupted", e);
} finally {
- if (metricsProvider != null) {
- try {
- metricsProvider.stop();
- } catch (Throwable error) {
- LOG.warn("Error while stopping metrics", error);
- }
+ try {
Review comment:
Please make this variable *final*
----------------------------------------------------------------
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] ztzg closed pull request #1562: ZOOKEEPER-4033: Remove unnecessary judgment of null
Posted by GitBox <gi...@apache.org>.
ztzg closed pull request #1562:
URL: https://github.com/apache/zookeeper/pull/1562
----------------------------------------------------------------
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