You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by dkesler <gi...@git.apache.org> on 2015/02/09 22:22:15 UTC
[GitHub] curator pull request: Curator 187
GitHub user dkesler opened a pull request:
https://github.com/apache/curator/pull/66
Curator 187
A fix for the problem described in https://issues.apache.org/jira/browse/CURATOR-187. The ChildReaper continuously runs even when its Reaper is not the leader, resulting in a pileup of paths to reap in the Reaper's queue.
This changeset allows the ChildReaper to create the LeaderLatch so that it too can avoid doing any work (i.e. adding paths to the Reaper's queue) if this application is not the leader.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yodle/curator CURATOR-187
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/66.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #66
----
commit 520ae54ac4a49292201417fa6b1104cf579704d3
Author: David Kesler <dk...@yodle.com>
Date: 2015-02-09T18:19:20Z
Adding a new constructor to Reaper so that it can optionally take a fully constructed leader latch that is owned by another class rather than create its own leader latch
commit 9da79604bfe7e775d1d42a25f0ac7579b8ee0464
Author: David Kesler <dk...@yodle.com>
Date: 2015-02-09T18:25:51Z
Reaper now sets its initial reapingIsActive state based on the state of the leader latch in case the leader latch was already started before being passed into the reaper's constructor
commit 49eb02a04f377a3b9e2da3b3904311ddddf1aa9d
Author: David Kesler <dk...@yodle.com>
Date: 2015-02-09T18:50:39Z
ChildReaper now creates a leaderLatch itself if a leader path is provided and does no work (such as passing paths to its Reaper) if it is not currently the leader.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by dkesler <gi...@git.apache.org>.
Github user dkesler commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24378616
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/Reaper.java ---
@@ -333,7 +355,20 @@ public void notLeader()
reapingIsActive.set(false);
}
};
- localLeaderLatch.addListener(listener);
- return localLeaderLatch;
+ leaderLatch.addListener(listener);
+
+ reapingIsActive.set(leaderLatch.hasLeadership());
+ }
+
+ private static LeaderLatch makeLeaderLatchIfPathNotNull(CuratorFramework client, String leaderPath)
--- End diff --
It's because it's called from within a constructor that calls another constructor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by dkesler <gi...@git.apache.org>.
Github user dkesler commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24379974
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/ChildReaper.java ---
@@ -173,32 +191,40 @@ public boolean removePath(String path)
return paths.remove(PathUtils.validatePath(path));
}
- private static ScheduledExecutorService newExecutorService()
+ public static ScheduledExecutorService newExecutorService()
{
return ThreadUtils.newFixedThreadScheduledPool(2, "ChildReaper");
}
private void doWork()
{
- for ( String path : paths )
+ if (shouldDoWork())
--- End diff --
That race condition is already present with the leader election in the Reaper itself as it could lose leadership after scheduling a check for some time in the future, so this is at least no worse than the current state of things. And the race condition doesn't matter too much anyway as the reapers don't use leader election for correctness, but rather because otherwise you get a bunch of logspam about trying to delete already deleted nodes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/66
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24379660
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/Reaper.java ---
@@ -333,7 +355,20 @@ public void notLeader()
reapingIsActive.set(false);
}
};
- localLeaderLatch.addListener(listener);
- return localLeaderLatch;
+ leaderLatch.addListener(listener);
+
+ reapingIsActive.set(leaderLatch.hasLeadership());
--- End diff --
Ok.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24375761
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/Reaper.java ---
@@ -333,7 +355,20 @@ public void notLeader()
reapingIsActive.set(false);
}
};
- localLeaderLatch.addListener(listener);
- return localLeaderLatch;
+ leaderLatch.addListener(listener);
+
+ reapingIsActive.set(leaderLatch.hasLeadership());
--- End diff --
Another potential race condition here? If the Reaper gets passed a LeaderLatch in its constructor that has already undergone election (and been elected leader) then the isLeader() method on the listener will not fire until another election occurs. While the reapiningIsActive flag will be set to true, the schedule() method will not get called so no reaping will actually occur?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24380304
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/ChildReaper.java ---
@@ -173,32 +191,40 @@ public boolean removePath(String path)
return paths.remove(PathUtils.validatePath(path));
}
- private static ScheduledExecutorService newExecutorService()
+ public static ScheduledExecutorService newExecutorService()
{
return ThreadUtils.newFixedThreadScheduledPool(2, "ChildReaper");
}
private void doWork()
{
- for ( String path : paths )
+ if (shouldDoWork())
--- End diff --
Yep, thought as much.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24378861
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/Reaper.java ---
@@ -333,7 +355,20 @@ public void notLeader()
reapingIsActive.set(false);
}
};
- localLeaderLatch.addListener(listener);
- return localLeaderLatch;
+ leaderLatch.addListener(listener);
+
+ reapingIsActive.set(leaderLatch.hasLeadership());
+ }
+
+ private static LeaderLatch makeLeaderLatchIfPathNotNull(CuratorFramework client, String leaderPath)
--- End diff --
Yep, good point, didn't look at where it was being called from.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/66#issuecomment-73622559
Looks good to me. Thanks for the fix! I will merge shortly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by dkesler <gi...@git.apache.org>.
Github user dkesler commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24378993
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/Reaper.java ---
@@ -333,7 +355,20 @@ public void notLeader()
reapingIsActive.set(false);
}
};
- localLeaderLatch.addListener(listener);
- return localLeaderLatch;
+ leaderLatch.addListener(listener);
+
+ reapingIsActive.set(leaderLatch.hasLeadership());
--- End diff --
schedule() gets called on the individual paths in activePaths. Since this method only gets called during the construction of the Reaper, there won't be any activePaths to schedule. When those paths get added later, they will get scheduled as expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24375781
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/Reaper.java ---
@@ -333,7 +355,20 @@ public void notLeader()
reapingIsActive.set(false);
}
};
- localLeaderLatch.addListener(listener);
- return localLeaderLatch;
+ leaderLatch.addListener(listener);
+
+ reapingIsActive.set(leaderLatch.hasLeadership());
+ }
+
+ private static LeaderLatch makeLeaderLatchIfPathNotNull(CuratorFramework client, String leaderPath)
--- End diff --
Any reason for this to be static?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Curator 187
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/66#discussion_r24375263
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/ChildReaper.java ---
@@ -173,32 +191,40 @@ public boolean removePath(String path)
return paths.remove(PathUtils.validatePath(path));
}
- private static ScheduledExecutorService newExecutorService()
+ public static ScheduledExecutorService newExecutorService()
{
return ThreadUtils.newFixedThreadScheduledPool(2, "ChildReaper");
}
private void doWork()
{
- for ( String path : paths )
+ if (shouldDoWork())
--- End diff --
There is potential for a race condition here if leadership is lost while attempting to do the reaping. Not sure if this is an issue or not?
I would assume that it's not really an issue as:
1.) If leadership has been lost then the connection to ZK will be lost so the reaping won't work anyway.
2.) If the reaping fails due to 2 nodes concurrently attempting to delete / query children, then it will just run again next time anyway.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---