[helix] branch master updated: Move subscribeForChange out of critical section (#1540)

[ji...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jiajunwang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git


The following commit(s) were added to refs/heads/master by this push:
     new dac8582  Move subscribeForChange out of critical section (#1540)
dac8582 is described below

commit dac85828f59ca4ef1fee97bce5ba8d380b98b659
Author: xyuanlu <xy...@gmail.com>
AuthorDate: Thu Nov 19 11:07:34 2020 -0800

    Move subscribeForChange out of critical section (#1540)
    
    Move subscribeForChange out of critical section to solve lock contention.
---
 .../org/apache/helix/manager/zk/CallbackHandler.java | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java b/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
index a57a678..8342ef5 100644
--- a/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
+++ b/helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
@@ -340,26 +340,28 @@ public class CallbackHandler implements IZkChildListener, IZkDataListener {
   public void invoke(NotificationContext changeContext) throws Exception {
     Type type = changeContext.getType();
     long start = System.currentTimeMillis();
+    if (logger.isInfoEnabled()) {
+      logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+          Thread.currentThread().getId(), _uid, _path, _listener, type);
+    }
 
-    // This allows the listener to work with one change at a time
-    synchronized (_manager) {
-      if (logger.isInfoEnabled()) {
-        logger
-            .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
-                _uid, _path, _listener, type);
-      }
-
+    synchronized (this) {
       if (!_expectTypes.contains(type)) {
         logger.warn("Callback handler {} received event in wrong order. Listener: {}, path: {}, "
             + "expected types: {}, but was {}", _uid, _listener, _path, _expectTypes, type);
         return;
-
       }
       _expectTypes = nextNotificationType.get(type);
 
       if (type == Type.INIT || type == Type.FINALIZE || changeContext.getIsChildChange()) {
         subscribeForChanges(changeContext.getType(), _path, _watchChild);
       }
+    }
+
+    // This allows the Helix Manager to work with one change at a time
+    // TODO: Maybe we don't need to sync on _manager for all types of listener. PCould be a
+    // potential improvement candidate.
+    synchronized (_manager) {
       if (_changeType == IDEAL_STATE) {
         IdealStateChangeListener idealStateChangeListener = (IdealStateChangeListener) _listener;
         List<IdealState> idealStates = preFetch(_propertyKey);