XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r957341820
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
{
try
{
- if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED)
|| !hasLeadership.get() )
Review Comment:
I'm wondering whether we should remove this `if` entirely: Let's assume the
current instance doesn't have the leadership but watches the child node
referring to the current leader. If the connection was temporarily suspended it
could be that the actual leader lost its leadership in the meantime. In that
case, we want the current instance to check the leadership. :thinking:
##########
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##########
@@ -218,6 +218,30 @@ public void testWatchedNodeDeletedOnReconnect() throws
Exception
}
}
+ @Test
+ public void testOurPathDeletedOnReconnect() throws Exception
Review Comment:
The test is not suitable for the change. I reverted the change and ran the
test. The test still succeeded.
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
{
try
{
- if (
client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED)
|| !hasLeadership.get() )
Review Comment:
But thinking about it once more, this might not be an issue because the
current LeaderLatch would have a watcher on the current leader's node which
would have been triggered when the leader's child would have been deleted.
:thinking:
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -604,7 +604,7 @@ else if ( ourIndex == 0 )
@Override
public void process(WatchedEvent event)
{
- if ( (state.get() == State.STARTED) && (event.getType() ==
Event.EventType.NodeDeleted) && (localOurPath != null) )
Review Comment:
Good point. Due to the condition in
[LeaderLatch:588](https://github.com/apache/curator/blob/425598cb6bf6a5b227a5fdd293fe46c7978d6578/curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java#L588),
`ourIndex` must be `-1` if `localOurPath` is actually `null` and, therefore,
would prevent the execution from reaching the `else` branch (instead the if
condition in
[LeaderLatch:592](https://github.com/apache/curator/blob/425598cb6bf6a5b227a5fdd293fe46c7978d6578/curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java#L592)
applies) where the watcher is set.
--
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
