[GitHub] [druid] kfaraz commented on a diff in pull request #13251: Adding new state to leader election

[GitBox <gi...@apache.org>]

kfaraz commented on code in PR #13251:
URL: https://github.com/apache/druid/pull/13251#discussion_r1001765586


##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskMaster.java:
##########
@@ -236,8 +227,10 @@ public Optional<String> getRedirectLocation()
     String leader = overlordLeaderSelector.getCurrentLeader();
     // do not redirect when
     // leader is not elected
-    // leader is the current node
-    if (leader == null || leader.isEmpty() || overlordLeaderSelector.isLeader()) {
+    // leader is the current node but not intialized.
+    if (leader == null
+        || leader.isEmpty()
+        || overlordLeaderSelector.isLeader() == DruidLeaderSelector.LeaderState.ELECTED) {

Review Comment:
   Shouldn't this also include the case when `isLeader` returns `INITIALIZED`?



##########
server/src/main/java/org/apache/druid/rpc/ServiceClient.java:
##########
@@ -37,7 +37,7 @@
  */
 public interface ServiceClient
 {
-  long MAX_REDIRECTS = 3;
+  long MAX_REDIRECTS = 5;

Review Comment:
   Maybe add a comment on why we need this value at 5.



##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java:
##########
@@ -731,7 +759,7 @@ private void stopBeingLeader()
       lookupCoordinatorManager.stop();
       metadataRuleManager.stop();
       segmentsMetadataManager.stopPollingDatabasePeriodically();
-
+      exec.shutdownNow();

Review Comment:
   This need not be shutdown here as each `DutiesRunnable` which runs on this executor individually checks whether it should REPEAT or STOP.
   If we do want to shutdown to be sure, however, then we must also re-initialize it in `becomeLeader`.



##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskMaster.java:
##########
@@ -170,7 +161,7 @@ public void stopBeingLeader()
       {
         giant.lock();
         try {
-          initialized = false;

Review Comment:
   Doesn't removing this mean that we miss out on the state of initialization of this class itself?



##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java:
##########
@@ -873,10 +901,14 @@ public void run()
       try {
         final long globalStart = System.nanoTime();
         synchronized (lock) {
-          if (!coordLeaderSelector.isLeader()) {
-            log.info("LEGGO MY EGGO. [%s] is leader.", coordLeaderSelector.getCurrentLeader());
+          DruidLeaderSelector.LeaderState leaderState = coordLeaderSelector.isLeader();
+          if (leaderState == DruidLeaderSelector.LeaderState.NOT_ELECTED) {
+            log.info("I am no longer the leader. Current leader is [%s].", coordLeaderSelector.getCurrentLeader());
             stopBeingLeader();
             return;
+          } else if (leaderState == DruidLeaderSelector.LeaderState.ELECTED) {
+            throw new ISE(

Review Comment:
   Better to just log an error here and return.
   An exception thrown here is just caught, logged and swallowed in `ScheduledExecutors.scheduledAtFixedRate()`.
   



-- 
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: commits-unsubscribe@druid.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org