CURATOR-696. Fix double leader for LeaderLatch #500
Conversation
Signed-off-by: tison <[email protected]>
| final long ephemeralOwner = | ||
| event.getStat() != null ? event.getStat().getEphemeralOwner() : -1; | ||
| final long thisSessionId = | ||
| client.getZookeeperClient().getZooKeeper().getSessionId(); | ||
| if (ephemeralOwner != thisSessionId) { | ||
| // this node is gone - reset | ||
| reset(); | ||
| } else { | ||
| lastPathIsLeader.set(localOurPath); | ||
| setLeadership(true); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: tison <[email protected]>
|
@kezhuw Thanks for your review.
Curator has a basic assumption that we own the node (TN-7). This patch fixes a bug that we don't manage the node (ephemeral owner, a.k.a, session id) properly. We may not handle "external deletion". And such a watcher doesn't fix our issue here, because it can take some time the deletion pass to the client before it gives up the leadership. |
Signed-off-by: tison <[email protected]>
|
@eolivelli pushed. Ensure that it fails without this patch and passes with this patch. |
Reproduced with a more handy tests. The test pushed above can prevent regression already. |
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java
Outdated
Show resolved
Hide resolved
|
I'll merge this patch later this week if no more inputs. I'd still seek possibility to reduce the test consumption since it now takes 140+ minutes to finish ... |
|
@tisonkun do you know when the 5.7.0 release will be made available? |
|
@razinbouzar I'll start the discussion today and hopefully vote in two to three weeks. You can keep an eye on the dev mailing list ([email protected]). |
|
@razinbouzar Curator 5.7.0 has released. |
A possible fix to CURATOR-696.
I'm thinking whether this has more corner cases ...
cc @gianm @kezhuw @eolivelli