[GitHub] [helix] dasahcc commented on a change in pull request #1149: Refine the ZkHelixManager.disconnect() method so it won't be blocked when the ZK connection breaks.

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

dasahcc commented on a change in pull request #1149:
URL: https://github.com/apache/helix/pull/1149#discussion_r455956001



##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -866,6 +858,83 @@ public void disconnect() {
     }
   }
 
+  /**
+   * The callback handler cleanup operations that require an active ZkClient connection.

Review comment:
       Why this require an active zkclient? CallbackHandler are in memory object, is there anything we need to connection to ZK?

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -866,6 +858,83 @@ public void disconnect() {
     }
   }
 
+  /**
+   * The callback handler cleanup operations that require an active ZkClient connection.
+   * If ZkClient is not connected, Helix Manager shall skip the cleanup.
+   *
+   * @return true if the cleanup has been done successfully.
+   */
+  private boolean cleanupCallbackHandlers() {
+    AtomicBoolean cleanupDone = new AtomicBoolean(false);
+
+    if (_zkclient.waitUntilConnected(_waitForConnectedTimeout, TimeUnit.MILLISECONDS)) {
+      // Create a separate thread for executing cleanup task to avoid forever retry.
+      Thread cleanupThread = new Thread(String
+          .format("Cleanup thread for %s-%s-%s", _clusterName, _instanceName, _instanceType)) {
+        @Override
+        public void run() {
+          // TODO reset user defined handlers only
+          resetHandlers(true);
+
+          if (_leaderElectionHandler != null) {
+            _leaderElectionHandler.reset(true);
+          }
+
+          ParticipantManager participantManager = _participantManager;
+          if (participantManager != null) {
+            participantManager.disconnect();
+          }
+
+          cleanupDone.set(true);
+        }
+      };
+
+      // Define the state listener to terminate the cleanup thread when the ZkConnection breaks.
+      IZkStateListener stateListener = new IZkStateListener() {

Review comment:
       It is complicated to create a listener for subscription. Shall we use some simple way to see whether the connection is alive? For example, just have an exist call for the cluster since it is in HelixManager?




----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org