[GitHub] [curator] kezhuw commented on a diff in pull request #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close

["kezhuw (via GitHub)" <gi...@apache.org>]

kezhuw commented on code in PR #481:
URL: https://github.com/apache/curator/pull/481#discussion_r1330087028


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -200,8 +200,6 @@ private synchronized void internalClose(CloseMode closeMode, boolean failOnClose
             }
         }
 
-        Preconditions.checkNotNull(closeMode, "closeMode cannot be null");

Review Comment:
   I think there are three candidates in case of `null` `closeMode` in `close(CloseMode closeMode)`:
   1. Throw NPE  before state change in `close(CloseMode closeMode)`, just as what the jira reporter suggest.
   2. Default to `this.closeMode` just as no argument version `close()`.
   3. Default to `CloseMode.SILENT`.
   
   I think we should resort to first two but not the third as the "default behavior" should apply to constructor but not `close`. I will only prefer to `CloseMode.SILENT` if there is no `this.closeMode`.



##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -213,18 +211,12 @@ private synchronized void internalClose(CloseMode closeMode, boolean failOnClose
         } finally {
             client.getConnectionStateListenable().removeListener(listener);
 
-            switch (closeMode) {
-                case NOTIFY_LEADER: {
-                    setLeadership(false);
-                    listeners.clear();
-                    break;
-                }
-
-                default: {
-                    listeners.clear();
-                    setLeadership(false);
-                    break;
-                }
+            if (closeMode == CloseMode.NOTIFY_LEADER) {

Review Comment:
   I think it is better to choose `closeMode` in the first place(a.k.a. `close(CloseMode closeMode)`), but not here. I just googled to know that `switch(null)` will cause NPE.



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

To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org