[
https://issues.apache.org/jira/browse/CURATOR-644?focusedWorklogId=809959&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-809959
]
ASF GitHub Bot logged work on CURATOR-644:
------------------------------------------
Author: ASF GitHub Bot
Created on: 19/Sep/22 07:58
Start Date: 19/Sep/22 07:58
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 809959)
Time Spent: 6h 20m (was: 6h 10m)
> CLONE - Race conditions in LeaderLatch after reconnecting to ensemble
> ---------------------------------------------------------------------
>
> Key: CURATOR-644
> URL: https://issues.apache.org/jira/browse/CURATOR-644
> Project: Apache Curator
> Issue Type: Bug
> Affects Versions: 4.2.0
> Reporter: Ken Huang
> Assignee: Jordan Zimmerman
> Priority: Minor
> Time Spent: 6h 20m
> Remaining Estimate: 0h
>
> Clone from CURATOR-504.
> We use LeaderLatch in a lot of places in our system and when ZooKeeper
> ensemble is unstable and clients are reconnecting to logs are full of
> messages like the following:
> {{{}[2017-08-31
> 19:18:34,562][ERROR][org.apache.curator.framework.recipes.leader.LeaderLatch]
> Can't find our node. Resetting. Index: -1 {{}}}}
> According to the
> [implementation|https://github.com/apache/curator/blob/4251fe328908e5fca37af034fabc190aa452c73f/curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java#L529-L536],
> this can happen in two cases:
> * When internal state `ourPath` is null
> * When the list of latches does not have the expected one.
> I believe we hit the first condition because of races that occur after client
> reconnects to ZooKeeper.
> * Client reconnects to ZooKeeper and LeaderLatch gets the event and calls
> reset method which set the internal state (`ourPath`) to null, removes old
> latch and creates a new one. This happens in thread
> "Curator-ConnectionStateManager-0".
> * Almost simultaneously, LeaderLatch gets another even NodeDeleted
> ([here|https://github.com/apache/curator/blob/4251fe328908e5fca37af034fabc190aa452c73f/curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java#L543-L554])
> and tries to re-read the list of latches and check leadership. This happens
> in the thread "main-EventThread".
> Therefore, sometimes there is a situation when method `checkLeadership` is
> called when `ourPath` is null.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
