xyuanlu commented on code in PR #2409:
URL: https://github.com/apache/helix/pull/2409#discussion_r1144092886


##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -266,18 +279,25 @@ public void asyncSet(String key, T data, int version, 
AsyncCallback.StatCallback
 
   @Override
   public void connect() {
-    // TODO: throws IllegalStateException when already connected
     try {
+      _zkClientConnectionMutex.lock();
       _zkClient.connect(_initConnectionTimeout, _zkClient);
+      // register this client as state change listener to react to ZkClient 
EXPIRED event.
+      // When ZkClient has expired connection to ZK, it sill auto reconnect 
until ZkClient
+      // is closed or connection re-established.
+      // We will need to close ZkClient when user set retry connection timeout.
+      _zkClient.subscribeStateChanges(this);

Review Comment:
   TFTR. This is a good point.
   As our offline discussed, since ZkClient has a single thread (_eventThread) 
handling all events, it is possible that one previous event handling callback 
blocking all following event being send out. This is a generic problem with 
single event thread. To coup with this, we
   1. Mentioned explicitly in document that user should not register blocking 
callbacks to client. (ZooKeeper document also suggested this)
   2. In Helix use case, all callbacks are just simply forward. 
   



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


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to