[GitHub] [curator] kezhuw commented on a diff in pull request #446: CURATOR-518: Fix LeaderSelector requeue broken by interruptLeadership

["kezhuw (via GitHub)" <gi...@apache.org>]

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: dev-unsubscribe@curator.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org