XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r973954972
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -190,6 +190,12 @@ public void close() throws IOException
close(closeMode);
}
+ // for testing
+ void closeOnDemand() throws IOException
+ {
+ internalClose(closeMode, false);
+ }
Review Comment:
For now, this is a test-only method. I'm a bit reluctant to add test-related
code in production code rather than providing a utilily class instead (to be
fair: we do that in curator code in other locations like the
`debug*CountDownLatches` in the `LeaderLatch` implementation). I see a risk if
convenient methods like this are available in production: They might make it
easier to write less-consistent production code. :thinking:
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -190,6 +190,12 @@ public void close() throws IOException
close(closeMode);
}
+ // for testing
Review Comment:
```suggestion
@VisibleForTesting
```
Isn't this annotation the better way of documenting this here?
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -190,6 +190,12 @@ public void close() throws IOException
close(closeMode);
}
+ // for testing
+ void closeOnDemand() throws IOException
+ {
+ internalClose(closeMode, false);
+ }
Review Comment:
...but if you want to stick to it, I would vote for coming up with a more
descriptive name for it: Something like `closeIfStillRunning`
##########
curator-test-zk35/pom.xml:
##########
@@ -166,6 +166,12 @@
<artifactId>slf4j-log4j12</artifactId>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>org.awaitility</groupId>
+ <artifactId>awaitility</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Why is this change necessary?
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -198,9 +204,25 @@ public void close() throws IOException
* @param closeMode allows the default close mode to be overridden at the
time the latch is closed.
* @throws IOException errors
*/
- public synchronized void close(CloseMode closeMode) throws IOException
+ public void close(CloseMode closeMode) throws IOException
{
- Preconditions.checkState(state.compareAndSet(State.STARTED,
State.CLOSED), "Already closed or has not been started");
+ internalClose(closeMode, true);
+ }
+
+ private synchronized void internalClose(CloseMode closeMode, boolean
failOnClosed) throws IOException
+ {
+ if (!state.compareAndSet(State.STARTED, State.CLOSED))
+ {
+ if (failOnClosed)
+ {
+ throw new IllegalStateException("Already closed or has not
been started");
+ }
+ else
+ {
+ return;
+ }
Review Comment:
```suggestion
return;
```
nit: Is the `else` branch required based on code guidelines? If not, I'd
propose removing it to shorten the code. The code format (having opening
brackets on a new line) makes the code long, anyway already.
--
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: dev-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
