[GitHub] [helix] dasahcc commented on a change in pull request #376: Add #batchGetInstancesStoppableChecks to solve performance issue

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

dasahcc commented on a change in pull request #376: Add #batchGetInstancesStoppableChecks to solve performance issue
URL: https://github.com/apache/helix/pull/376#discussion_r311259806
 
 

 ##########
 File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
 ##########
 @@ -122,115 +122,166 @@ public InstanceInfo getInstanceInfo(String clusterId, String instanceName,
 
   /**
    * {@inheritDoc}
+   * Single instance stoppable check implementation is a special case of
+   * {@link #batchGetInstancesStoppableChecks(String, List, String)}
+   *
    * Step 1: Perform instance level Helix own health checks
    * Step 2: Perform instance level client side health checks
    * Step 3: Perform partition level (all partitions on the instance) client side health checks
-   * Note: if the check fails at one step, all the following steps won't be executed because the instance cannot be stopped
+   *
+   * Note: if the check fails at one step, the rest steps won't be executed because the instance cannot be stopped
    */
   @Override
-  public StoppableCheck getInstanceStoppableCheck(String clusterId, String instanceName,
+  public StoppableCheck getInstanceStoppableCheck(String clusterId, String instanceName, String jsonContent)
+      throws IOException {
+    return batchGetInstancesStoppableChecks(clusterId, ImmutableList.of(instanceName), jsonContent).get(instanceName);
+  }
+
+  @Override
+  public Map<String, StoppableCheck> batchGetInstancesStoppableChecks(String clusterId, List<String> instances,
       String jsonContent) throws IOException {
-    LOG.info("Perform instance level helix own health checks for {}/{}", clusterId, instanceName);
-    Map<String, Boolean> helixStoppableCheck = getInstanceHealthStatus(clusterId, instanceName,
-        InstanceService.HealthCheck.STOPPABLE_CHECK_LIST);
-
-    StoppableCheck result =
-        new StoppableCheck(helixStoppableCheck, StoppableCheck.Category.HELIX_OWN_CHECK);
-    if (!result.isStoppable()) {
-      return result;
+    Map<String, StoppableCheck> finalStoppableChecks = new HashMap<>();
+    Map<String, Future<StoppableCheck>> helixInstanceChecks = instances.stream()
 
 Review comment:
   I think you need to add all instances keys to the finalStoppableChecks. Otherwise, it could return an empty map when we RESTConfig is not setup. 
   
   The stoppable instances is also checked from final mapping. So either you change the top level result preparing or change the logic here.

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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org