[GitHub] [helix] jiajunwang commented on a change in pull request #1540: Move subscribeForChange out of critical section

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

jiajunwang commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r525619919



##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
   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 listener to work with one change at a time

Review comment:
       nit, "the listener" -> "the Helix Manager"?
   Also, let's add a TODO here since it might be overkill as we discussed.




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