[GitHub] [helix] junkaixue commented on a diff in pull request #2183: Optimize HelixTaskExecutor reset() in event of shutdown

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

junkaixue commented on code in PR #2183:
URL: https://github.com/apache/helix/pull/2183#discussion_r942803893


##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -734,6 +744,7 @@ void init() {
     }
 
     _isShuttingDown = false;
+    _isResetComplete = false;

Review Comment:
   Why not put in onMessage? OnMessage is the place starting STs and changing the states.



##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -827,7 +838,11 @@ public void onMessage(String instanceName, List<Message> messages,
     // and terminate all the thread pools
     // TODO: see if we should have a separate notification call for resetting
     if (changeContext.getType() == Type.FINALIZE) {
-      reset();
+      if (_isShuttingDown) {

Review Comment:
   Since we already use new flag to protect it. Let's not use this isShutdowning to do another filtering.
   
   There could be race condition happening. The flag started with flag and if there is a session timeout and restart everything. Before this flag set to true in the registryFactory, you get finalize message from ZK server. You will bypass the reset and use the old data.
   
   If we already decide to use lowest component to do dedup, let's not add extra optimization in up layer, which may have potential issues.
   



##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -237,6 +239,7 @@ private void registerMessageHandlerFactory(String type, MessageHandlerFactory fa
     }
 
     _isShuttingDown = false;
+    _isResetComplete = false;

Review Comment:
   NIT: this is more like an action flag instead of state flag. Personally, I prefer flag like isCleanState or something else.



##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -694,16 +701,19 @@ void reset() {
           LOG.info("Reset executor for msgType: " + msgType + ", pool: " + pool);
           shutdownAndAwaitTermination(pool, item);
         }
-
-        if (item.factory() != null) {
-          try {
-            item.factory().reset();
-          } catch (Exception ex) {
-            LOG.error("Failed to reset the factory {} of message type {}.", item.factory().toString(),
-                msgType, ex);
-          }
-        }
       }
+      _hdlrFtyRegistry.values()
+          .parallelStream()
+          .map(MsgHandlerFactoryRegistryItem::factory)
+          .distinct()
+          .filter(Objects::nonNull)
+          .forEach(factory -> {
+            try {
+              factory.reset();
+            } catch (Exception ex) {
+              LOG.error("Failed to reset the factory {}.", factory.toString(), ex);
+            }
+          });

Review Comment:
   I would suggest to make parallel reset in a different PR. This requires more thinking. 1) we cannot guarantee there is not dependencies and user are not leveraging dependencies because of sequential reset. 2) if there is a problem for parallel reset, you can just revert reset PR instead of entire PR to revert dedup logic.



-- 
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: reviews-unsubscribe@helix.apache.org

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