kezhuw commented on code in PR #446:
URL: https://github.com/apache/curator/pull/446#discussion_r1125447099
##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderSelector.java:
##########
@@ -237,13 +242,15 @@ private synchronized boolean internalRequeue()
if ( !isQueued && (state.get() == State.STARTED) )
{
isQueued = true;
- Future<Void> task = executorService.submit(new Callable<Void>()
+ Future<?> oldTask = ourTask;
+ ourTask = executorService.submit(new Callable<Void>()
{
@Override
public Void call() throws Exception
{
try
{
+ waitTaskDone(oldTask, Duration.ofMillis(500),
Duration.ofSeconds(5));
Review Comment:
I thought about the guarantee `LeaderSelector` try to provide, and conclude
to that:
* It is crucial for `LeaderSelectorListener.takeLeadership` to not swallow
interruption silently without return. Otherwise, states(e.g. `hasLeadership`,
`mutex`) could be changed by dated task.
* Interruption for `LeaderSelectorListener.takeLeadership` is a must
regardless of leadership. Otherwise, `LeaderSelectorListener.takeLeadership`
could run with dated leadership and no future interruption.
* Without `autoRequeue`, `requeue` is hard to use.
* An attempt could be cancelled due to session state change.
* `requeue` after `LeaderSelector.interruptLeadership` might be a noop.
For your questions:
1. Yes. It may be better to enforce only one
`LeaderSelectorListener.takeLeadership` call simultaneously. But that could
make this pr not concentrated.
2. Yes, but `LeaderSelector.interruptLeadership` is not enough as it could
introduce dated leadership.
I think I messed up things here, it might be better to make this pr more
concentrated on CURATOR-518 and leaves other concerns(concurrency of
`LeaderSelector.doWork` and `requeue` usage) in followups.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
