You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@curator.apache.org by "tisonkun (via GitHub)" <gi...@apache.org> on 2023/09/15 14:14:20 UTC
[GitHub] [curator] tisonkun opened a new pull request, #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close
tisonkun opened a new pull request, #481:
URL: https://github.com/apache/curator/pull/481
(no comment)
--
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
[GitHub] [curator] kezhuw commented on a diff in pull request #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close
Posted by "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
[GitHub] [curator] tisonkun commented on pull request #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #481:
URL: https://github.com/apache/curator/pull/481#issuecomment-1725904616
@kezhuw Thanks for your review! I just realize that I totally misunderstand the original issue and update 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.
To unsubscribe, e-mail: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #481:
URL: https://github.com/apache/curator/pull/481#discussion_r1327348419
##########
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:
Ensure nonnulll and even we can fallback to the default behavior anyway.
##########
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:
Ensure nonnull and even we can fallback to the default behavior anyway.
--
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
[GitHub] [curator] kezhuw commented on a diff in pull request #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #481:
URL: https://github.com/apache/curator/pull/481#discussion_r1330099763
##########
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:
It is just a matter of style. Ignore if you have some preferences.
--
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
[GitHub] [curator] tisonkun commented on a diff in pull request #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #481:
URL: https://github.com/apache/curator/pull/481#discussion_r1327348419
##########
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:
Ensured nonnull and even we can fallback to the default behavior anyway.
--
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
[GitHub] [curator] tisonkun commented on a diff in pull request #481: CURATOR-521. Polish code of handling closeMode in LeaderLatch#close
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #481:
URL: https://github.com/apache/curator/pull/481#discussion_r1327348419
##########
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:
Ensured nonnull and even we can fallback to the default behavior anyway with changes below.
--
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
[GitHub] [curator] tisonkun merged pull request #481: CURATOR-521. Check LeaderLatch#close(CloseMode) does not accept null
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #481:
URL: https://github.com/apache/curator/pull/481
--
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