[GitHub] [helix] qqu0127 commented on a diff in pull request #2390: Add API for users to provide customized threadpool for different categories of state transition messages

["qqu0127 (via GitHub)" <gi...@apache.org>]

qqu0127 commented on code in PR #2390:
URL: https://github.com/apache/helix/pull/2390#discussion_r1124680876


##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -361,17 +374,13 @@ ExecutorService findExecutorServiceForMsg(Message message) {
         executorService = _batchMessageExecutorService;
       } else {
         Message.MessageInfo msgInfo = new Message.MessageInfo(message);
-        String perResourceTypeKey =
-            msgInfo.getMessageIdentifier(Message.MessageInfo.MessageIdentifierBase.PER_RESOURCE);
-        String perStateTransitionTypeKey =
-            msgInfo.getMessageIdentifier(Message.MessageInfo.MessageIdentifierBase.PER_STATE_TRANSITION_TYPE);
-        if (perStateTransitionTypeKey != null && _executorMap.containsKey(perStateTransitionTypeKey)) {
-          LOG.info(String.format("Find per state transition type thread pool for resource %s from %s to %s",
-              message.getResourceName(), message.getFromState(), message.getToState()));
-          executorService = _executorMap.get(perStateTransitionTypeKey);
-        } else if (_executorMap.containsKey(perResourceTypeKey)) {
-          LOG.info("Find per-resource thread pool with key: " + perResourceTypeKey);
-          executorService = _executorMap.get(perResourceTypeKey);
+        for (int i = Message.MessageInfo.MessageIdentifierBase.values().length - 1; i >= 0; i--) {
+          String msgIdentifer = msgInfo.getMessageIdentifier(Message.MessageInfo.MessageIdentifierBase.values()[i]);
+          if (msgIdentifer != null && _executorMap.containsKey(msgIdentifer)) {
+            LOG.info(String.format("Find customized threadpool for %s", msgIdentifer));
+            executorService = _executorMap.get(msgIdentifer);
+            break;

Review Comment:
   Curious why do we iterate in reverse order? Can we change the order of declaration in the Enum?



##########
helix-core/src/main/java/org/apache/helix/participant/statemachine/StateModelFactory.java:
##########
@@ -260,4 +261,42 @@ public ExecutorService getExecutorService(String resourceName) {
   public ExecutorService getExecutorService(String resourceName, String fromState, String toState) {
     return null;
   }
+
+  public static class CustomizedExecutorService {
+    private Message.MessageInfo.MessageIdentifierBase _base;
+    private ExecutorService _executorService;

Review Comment:
   These two can be final



##########
helix-core/src/main/java/org/apache/helix/model/Message.java:
##########
@@ -115,6 +116,11 @@ public enum MessageState {
     UNPROCESSABLE // get exception when create handler
   }
 
+  public enum STRebalanceType {
+    LOAD_REBALANCE,
+    RECOVERY_REBALANCE
+  }
+

Review Comment:
   Not an issue, just a thought. I think there is existing `RebalanceType` in a throttling related class. For widely used and fundamental enum like this, maybe it deserves a separate class and reduce duplication



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