[GitHub] [helix] NealSun96 commented on a diff in pull request #2176: Wire in the logic for TaskExecutor reset timeout parameter

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

NealSun96 commented on code in PR #2176:
URL: https://github.com/apache/helix/pull/2176#discussion_r921443165


##########
helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java:
##########
@@ -61,6 +61,22 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
     return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
   }
 
+  /**
+   * Construct a zk-based cluster manager that enforces all types (PARTICIPANT, CONTROLLER, and
+   * SPECTATOR) to have a name
+   * @param clusterName
+   * @param instanceName
+   * @param type
+   * @param zkAddr
+   * @param stateListener
+   * @param helixManagerProperty
+   * @return a HelixManager backed by Zookeeper
+   */
+  public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+      InstanceType type, String zkAddr, HelixManagerStateListener stateListener, HelixManagerProperty helixManagerProperty) {
+    return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener, helixManagerProperty);

Review Comment:
   I have concerns over this constructor, and also storing the reset timeout in HelixManagerProperty in general:
   
   If you check the ZkHelixManager logic, you can see that `zkAddr` and `helixManagerProperty.getZkConnectionConfig()` cannot coexist. This is why HelixManagerProperty is only provided in the constructor below this new one: the `zkAddr` in that constructor is null. Therefore, it concerns me that this constructor can lead to a lot of ZkHelixManager initialization failures because of the above mentioned conflict. I believe most users will provide both a zkAddr and a connection config. 



##########
helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java:
##########
@@ -54,12 +56,13 @@ public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloud
         helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
   }
 
-  private HelixManagerProperty(String version, long healthReportLatency,
+  private HelixManagerProperty(String version, long healthReportLatency, int msgHandlerResetTimeout,

Review Comment:
   Same as above: I have concerns over adding this timeout in HelixManagerProperty, due to the connectionConfig it carries. 



##########
helix-core/src/main/java/org/apache/helix/messaging/DefaultMessagingService.java:
##########
@@ -72,8 +78,8 @@ public DefaultMessagingService(HelixManager manager) {
         new ParticipantStatusMonitor(isParticipant, manager.getInstanceName()),
         new MessageQueueMonitor(manager.getClusterName(), manager.getInstanceName()));
     _asyncCallbackService = new AsyncCallbackService();
-    _taskExecutor.registerMessageHandlerFactory(MessageType.TASK_REPLY.name(),
-        _asyncCallbackService);
+    _taskExecutor.registerMessageHandlerFactory(_asyncCallbackService, TaskExecutor.DEFAULT_PARALLEL_TASKS, resetTimeoutMs);

Review Comment:
   I can answer for @qqu0127 : the new signature he used here checks for `_asyncCallbackService.getMessageTypes()` and calls the line 75 `registerMessageHandlerFactory` for each of the types. Since `_asyncCallbackService` only has one message type, which is `TASK_REPLY`, this call is identical with the old call, except that it also adds a resetTimeoutMS. 



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