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/11/03 13:33:46 UTC

[GitHub] [zookeeper] belugabehr opened a new pull request #1520: ZOOKEEPER-3988: Protect zkServer from NullPointerException

belugabehr opened a new pull request #1520:
URL: https://github.com/apache/zookeeper/pull/1520


   


----------------------------------------------------------------
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] belugabehr commented on pull request #1520: ZOOKEEPER-3988: Protect zkServer from NullPointerException

Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1520:
URL: https://github.com/apache/zookeeper/pull/1520#issuecomment-721174702


   @eolivelli For a primer on `Optional` check out:
   
   https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained#optional


----------------------------------------------------------------
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 #1520: ZOOKEEPER-3988: Protect zkServer from NullPointerException

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1520:
URL: https://github.com/apache/zookeeper/pull/1520#discussion_r516654842



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerCnxn.java
##########
@@ -147,8 +149,9 @@ public void incrOutstandingAndCheckThrottle(RequestHeader h) {
         if (h.getXid() <= 0) {
             return;
         }
-        if (zkServer.shouldThrottle(outstandingCount.incrementAndGet())) {
-            disableRecv(false);
+        if (zkServer.isPresent()

Review comment:
       I am not sure this is what we want,
   previously we would have seen an NPE here,
   now we aren't doing anything.
   
   This is just an example,
   can you please explain better the intent of the patch ?




----------------------------------------------------------------
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 #1520: ZOOKEEPER-3988: Protect zkServer from NullPointerException

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1520:
URL: https://github.com/apache/zookeeper/pull/1520#issuecomment-721181663


   @belugabehr thanks for the pointer. I know about Optional benefits.
   My point is that I don't find the new code much better than the code without the patch, also it looks contains more boilerplate than before.
   
   Let me think more about it


----------------------------------------------------------------
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] belugabehr commented on a change in pull request #1520: ZOOKEEPER-3988: Protect zkServer from NullPointerException

Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1520:
URL: https://github.com/apache/zookeeper/pull/1520#discussion_r516728503



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerCnxn.java
##########
@@ -147,8 +149,9 @@ public void incrOutstandingAndCheckThrottle(RequestHeader h) {
         if (h.getXid() <= 0) {
             return;
         }
-        if (zkServer.shouldThrottle(outstandingCount.incrementAndGet())) {
-            disableRecv(false);
+        if (zkServer.isPresent()

Review comment:
       My intent was to remove the NPE.  It is valid that `zkServer` could be `null` and yet there is no check for it (please review the original JIRA).
   
   Is there something else that should be done here?  Comments make it clear that `zkServer` has a valid value of `null`.




----------------------------------------------------------------
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