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

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

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


##########
helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java:
##########
@@ -34,6 +35,7 @@ public class HelixManagerProperty {
   private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
   private String _version;
   private long _healthReportLatency;
+  private int _msgHandlerResetTimeout = TaskExecutor.DEFAULT_MSG_HANDLER_RESET_TIMEOUT_MS;

Review Comment:
   I feel this is how Helix and Task framework code is intermingled.  msgHandlerResetTimeout - is it specific to Task? or it is generic msgHandler?
   
   If it is generic, then having default in previous file makes sense. if it is specific to task, then variable name should be changed to indicate it is taskSpecific.
   As far as I know it is generic.



##########
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);
   }
 
+  /**

Review Comment:
   nit:  this comment is same as all other constructors. 
   What is the unique difference of this constructor needs to be documented?
   



##########
helix-core/src/test/java/org/apache/helix/integration/manager/TestParticipantManager.java:
##########
@@ -101,8 +103,13 @@ public void simpleIntegrationTest() throws Exception {
         "MasterSlave", true); // do rebalance
 
     String instanceName = "localhost_12918";
-    HelixManager participant =
-        new ZKHelixManager(_clusterName, instanceName, InstanceType.PARTICIPANT, ZK_ADDR);
+    int resetTimeout = 300;

Review Comment:
   is there any checks wrt what valid resetTimeout value can be? Is there any MaX??



##########
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:
   can you please explain this change?  as change description doesn't help in this case.



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