You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/07/31 19:24:03 UTC

[GitHub] [helix] dasahcc opened a new pull request #1197: Support option to skip direct ZK read for health check API

dasahcc opened a new pull request #1197:
URL: https://github.com/apache/helix/pull/1197


   ### Issues
   
   - [ ] My PR addresses the following Helix issues and references them in the PR description:
   
   This change support the option "skipZKRead" in multiple health check API call.
   If the option is true, Helix will directly query custom partition health from provided source instead of ZK read.
   It solves (#1196)
   
   ### Description
   
   - [ ] Here are some details about my PR, including screenshots of any UI changes:
   
   ### Tests
   
   - [ ] The following tests are written for this issue:
   
   TestInstanceService.testCustomPartitionCheckWithSkipZKRead
   
   - [ ] The following is the result of the "mvn test" command on the appropriate module:
   
   [INFO] Tests run: 164, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 53.755 s - in TestSuite
   [INFO]
   [INFO] Results:
   [INFO]
   [INFO] Tests run: 164, Failures: 0, Errors: 0, Skipped: 0
   [INFO]
   [INFO] ------------------------------------------------------------------------
   [INFO] BUILD SUCCESS
   [INFO] ------------------------------------------------------------------------
   [INFO] Total time: 01:00 min
   [INFO] Finished at: 2020-07-31T12:17:50-07:00
   [INFO] Final Memory: 30M/838M
   [INFO] ------------------------------------------------------------------------
   
   ### Commits
   
   - [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
     1. Subject is separated from body by a blank line
     1. Subject is limited to 50 characters (not including Jira issue reference)
     1. Subject does not end with a period
     1. Subject uses the imperative mood ("add", not "adding")
     1. Body wraps at 72 characters
     1. Body explains "what" and "why", not "how"
   
   ### Documentation (Optional)
   
   - [ ] In case of new functionality, my PR adds documentation in the following wiki page:
   
   (Link the GitHub wiki you added)
   
   ### Code Quality
   
   - [ ] My diff has been formatted using helix-style.xml 
   (helix-style-intellij.xml if IntelliJ IDE is used)
   


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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464576604



##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestInstanceService.java
##########
@@ -140,6 +151,70 @@ public void testGetInstanceStoppableCheckConnectionRefused() throws IOException
     Assert.assertEquals(actual.getFailedChecks().get(0), expectedFailedCheck);
   }
 
+  @Test
+  public void testCustomPartitionCheckWithSkipZKRead() throws IOException {
+    // Let ZK result is health, but http request is unhealthy.
+    // We expect the check fail if we skipZKRead.
+
+    String testPartition = "PARTITION_0";
+    String slibilingInstance = "instance0.linkedin.com_1236";
+    String jsonContent = "{\n" + "   \"param1\": \"value1\",\n" + "\"param2\": \"value2\"\n" + "}";

Review comment:
       Was for viewing purpose for each entry set a line. Let me format it.




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



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

[GitHub] [helix] narendly commented on pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1197:
URL: https://github.com/apache/helix/pull/1197#issuecomment-667815579


   @dasahcc Please follow the pull request description guidelines. It's recommended that we use the following keywords:
   
   close
   closes
   closed
   fix
   fixes
   fixed
   resolve
   resolves
   resolved
   
   when referring to the issue number. This is clearly outlined in our guidelines so that when the PR gets closed, the issue gets linked and closed as well. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
   


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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464206196



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -61,19 +61,32 @@
   private final HelixDataAccessorWrapper _dataAccessor;
   private final ConfigAccessor _configAccessor;
   private final CustomRestClient _customRestClient;
+  private boolean _skipZKRead;

Review comment:
       Should this field be `final`?




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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464206347



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -61,19 +61,32 @@
   private final HelixDataAccessorWrapper _dataAccessor;
   private final ConfigAccessor _configAccessor;
   private final CustomRestClient _customRestClient;
+  private boolean _skipZKRead;
 
   public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor) {
     _dataAccessor = dataAccessor;
     _configAccessor = configAccessor;
     _customRestClient = CustomRestClientFactory.get();
   }
 
+  public InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor, boolean skipZKRead) {
+    this(dataAccessor,configAccessor);
+    this._skipZKRead = skipZKRead;
+  }
+
   @VisibleForTesting
   InstanceServiceImpl(HelixDataAccessorWrapper dataAccessor, ConfigAccessor configAccessor,
       CustomRestClient customRestClient) {
+    this(dataAccessor, configAccessor, customRestClient, false);

Review comment:
       Could we remove this constructor since you added a more comprehensive one below?




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



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

[GitHub] [helix] pkuwm commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464035621



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -19,6 +19,7 @@
  * under the License.
  */
 
+import com.google.common.annotations.VisibleForTesting;

Review comment:
       Not used in this file?

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -64,22 +65,38 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
     _restClient = CustomRestClientFactory.get();
   }
 
+  public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClient customRestClient) {

Review comment:
       Could it use Interface instead: `ZKHelixDataAccessor` -> `HelixDataAccessor`?

##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
##########
@@ -172,7 +172,7 @@ public Response instancesOperations(@PathParam("clusterId") String clusterId,
         admin.enableInstance(clusterId, enableInstances, false);
         break;
       case stoppable:
-        return batchGetStoppableInstances(clusterId, node);
+        return batchGetStoppableInstances(clusterId, node, Boolean.valueOf(skipZKRead));

Review comment:
       Nit: since parameter is `boolean` type, `Boolean.valueOf` -> `Boolean.parseBoolean()`?

##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/TestPerInstanceAccessor.java
##########
@@ -33,9 +33,15 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import org.apache.helix.ConfigAccessor;
 import org.apache.helix.HelixDataAccessor;
 import org.apache.helix.HelixException;
 import org.apache.helix.TestHelper;
+import org.apache.helix.model.RESTConfig;

Review comment:
       Remove these unused imports?




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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464576311



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -61,19 +61,32 @@
   private final HelixDataAccessorWrapper _dataAccessor;
   private final ConfigAccessor _configAccessor;
   private final CustomRestClient _customRestClient;
+  private boolean _skipZKRead;

Review comment:
       Final requires constructor have redundant code. Tried to refactor, but there is no good solution. Otherwise, the new constructor need to do same assignment for each fields once.
   
   And there is no set during the process. I think it should be fine.




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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464206617



##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestInstanceService.java
##########
@@ -140,6 +151,70 @@ public void testGetInstanceStoppableCheckConnectionRefused() throws IOException
     Assert.assertEquals(actual.getFailedChecks().get(0), expectedFailedCheck);
   }
 
+  @Test
+  public void testCustomPartitionCheckWithSkipZKRead() throws IOException {
+    // Let ZK result is health, but http request is unhealthy.
+    // We expect the check fail if we skipZKRead.
+
+    String testPartition = "PARTITION_0";
+    String slibilingInstance = "instance0.linkedin.com_1236";

Review comment:
       Typo: siblingInstance




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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464577428



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -64,22 +65,38 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
     _restClient = CustomRestClientFactory.get();
   }
 
+  public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClient customRestClient) {

Review comment:
       I cannot remember the details. But there are some places use the method only for ZKHelixDataAccessor but not interface.




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



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

[GitHub] [helix] dasahcc commented on pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on pull request #1197:
URL: https://github.com/apache/helix/pull/1197#issuecomment-670127516


   This PR is ready to be merged.


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



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

[GitHub] [helix] dasahcc merged pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #1197:
URL: https://github.com/apache/helix/pull/1197


   


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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464683938



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -64,24 +63,47 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
     _restClient = CustomRestClientFactory.get();
   }
 
+  public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClient customRestClient) {
+    super(dataAccessor);
+    _restClient = customRestClient;
+  }
+
   public Map<String, Map<String, Boolean>> getAllPartitionsHealthOnLiveInstance(
       RESTConfig restConfig, Map<String, String> customPayLoads) {
+    return getAllPartitionsHealthOnLiveInstance(restConfig, customPayLoads, false);
+  }
+
+  /**
+   * Retrieve partition health status for each live instances combined with reading health partition report from ZK
+   * and customized REST API call.
+   *
+   * @param restConfig        restConfig for the cluster contains customize REST API endpoint
+   * @param customPayLoads    user passed in customized payloads
+   * @param skipZKRead        skip the ZK read if this flag is true
+   * @return                  A map of instance -> partition -> healthy or not (boolean).
+   */
+  public Map<String, Map<String, Boolean>> getAllPartitionsHealthOnLiveInstance(
+      RESTConfig restConfig, Map<String, String> customPayLoads, boolean skipZKRead) {
     // Only checks the instances are online with valid reports
     List<String> liveInstances = getChildNames(keyBuilder().liveInstances());
     // Make a parallel batch call for getting all healthreports from ZK.
-    List<HelixProperty> zkHealthReports = getProperty(liveInstances.stream()
-        .map(instance -> keyBuilder().healthReport(instance, PARTITION_HEALTH_KEY))
-        .collect(Collectors.toList()), false);
+    List<HelixProperty> zkHealthReports;
+    if (!skipZKRead) {
+      zkHealthReports = getProperty(liveInstances.stream()
+          .map(instance -> keyBuilder().healthReport(instance, PARTITION_HEALTH_KEY))
+          .collect(Collectors.toList()), false);
+    } else {
+      zkHealthReports =
+          liveInstances.stream().map(instance -> new HelixProperty(instance)).collect(Collectors.toList());
+    }
     Map<String, Future<Map<String, Boolean>>> parallelTasks = new HashMap<>();
     for (int i = 0; i < liveInstances.size(); i++) {
       String liveInstance = liveInstances.get(i);
-      Optional<ZNRecord> maybeHealthRecord =
-          Optional.ofNullable(zkHealthReports.get(i)).map(HelixProperty::getRecord);
-      parallelTasks.put(liveInstance,
-          POOL.submit(() -> maybeHealthRecord
-              .map(record -> getPartitionsHealth(liveInstance, record, restConfig, customPayLoads))
-              .orElseGet(() -> getHealthStatusFromRest(liveInstance, Collections.emptyList(),
-                  restConfig, customPayLoads))));
+      Optional<ZNRecord> maybeHealthRecord = Optional.ofNullable(zkHealthReports.get(i)).map(HelixProperty::getRecord);
+      parallelTasks.put(liveInstance, POOL.submit(() -> maybeHealthRecord.map(
+          record -> getPartitionsHealthFromCustomAPI(liveInstance, record, restConfig, customPayLoads, skipZKRead))
+          .orElseGet(
+              () -> getHealthStatusFromRest(liveInstance, Collections.emptyList(), restConfig, customPayLoads))));

Review comment:
       Because there are a couple of scenarios:
   1. Partitions read from ZK, they are all healthy. No expired partitions. Directly return the ZK partition data.
   2. Partitions read from ZK, there are some expired partitions, query SN to get latest update.
   3. Partitions skip read from ZK, no partition data from ZK and no expired data, query SN without any partition specific to get all partition health status.




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



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

[GitHub] [helix] zhangmeng916 commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464668238



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -64,24 +63,47 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
     _restClient = CustomRestClientFactory.get();
   }
 
+  public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClient customRestClient) {
+    super(dataAccessor);
+    _restClient = customRestClient;
+  }
+
   public Map<String, Map<String, Boolean>> getAllPartitionsHealthOnLiveInstance(
       RESTConfig restConfig, Map<String, String> customPayLoads) {
+    return getAllPartitionsHealthOnLiveInstance(restConfig, customPayLoads, false);
+  }
+
+  /**
+   * Retrieve partition health status for each live instances combined with reading health partition report from ZK
+   * and customized REST API call.
+   *
+   * @param restConfig        restConfig for the cluster contains customize REST API endpoint
+   * @param customPayLoads    user passed in customized payloads
+   * @param skipZKRead        skip the ZK read if this flag is true
+   * @return                  A map of instance -> partition -> healthy or not (boolean).
+   */
+  public Map<String, Map<String, Boolean>> getAllPartitionsHealthOnLiveInstance(
+      RESTConfig restConfig, Map<String, String> customPayLoads, boolean skipZKRead) {
     // Only checks the instances are online with valid reports
     List<String> liveInstances = getChildNames(keyBuilder().liveInstances());
     // Make a parallel batch call for getting all healthreports from ZK.
-    List<HelixProperty> zkHealthReports = getProperty(liveInstances.stream()
-        .map(instance -> keyBuilder().healthReport(instance, PARTITION_HEALTH_KEY))
-        .collect(Collectors.toList()), false);
+    List<HelixProperty> zkHealthReports;
+    if (!skipZKRead) {
+      zkHealthReports = getProperty(liveInstances.stream()
+          .map(instance -> keyBuilder().healthReport(instance, PARTITION_HEALTH_KEY))
+          .collect(Collectors.toList()), false);
+    } else {
+      zkHealthReports =
+          liveInstances.stream().map(instance -> new HelixProperty(instance)).collect(Collectors.toList());
+    }
     Map<String, Future<Map<String, Boolean>>> parallelTasks = new HashMap<>();
     for (int i = 0; i < liveInstances.size(); i++) {
       String liveInstance = liveInstances.get(i);
-      Optional<ZNRecord> maybeHealthRecord =
-          Optional.ofNullable(zkHealthReports.get(i)).map(HelixProperty::getRecord);
-      parallelTasks.put(liveInstance,
-          POOL.submit(() -> maybeHealthRecord
-              .map(record -> getPartitionsHealth(liveInstance, record, restConfig, customPayLoads))
-              .orElseGet(() -> getHealthStatusFromRest(liveInstance, Collections.emptyList(),
-                  restConfig, customPayLoads))));
+      Optional<ZNRecord> maybeHealthRecord = Optional.ofNullable(zkHealthReports.get(i)).map(HelixProperty::getRecord);
+      parallelTasks.put(liveInstance, POOL.submit(() -> maybeHealthRecord.map(
+          record -> getPartitionsHealthFromCustomAPI(liveInstance, record, restConfig, customPayLoads, skipZKRead))
+          .orElseGet(
+              () -> getHealthStatusFromRest(liveInstance, Collections.emptyList(), restConfig, customPayLoads))));

Review comment:
       Why we cannot combine this case together with the case where skipZKRead is set to true? Is that because backward compatibility make it harder to code? 




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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464205325



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -64,22 +65,38 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
     _restClient = CustomRestClientFactory.get();
   }
 
+  public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor, CustomRestClient customRestClient) {
+    super(dataAccessor);
+    _restClient = customRestClient;
+  }
+
   public Map<String, Map<String, Boolean>> getAllPartitionsHealthOnLiveInstance(
       RESTConfig restConfig, Map<String, String> customPayLoads) {
+    return getAllPartitionsHealthOnLiveInstance(restConfig, customPayLoads, false);
+  }
+
+  public Map<String, Map<String, Boolean>> getAllPartitionsHealthOnLiveInstance(

Review comment:
       Add a JavaDoc for a public method. Here, we should explain what skipZkRead parameter does, and its implications.




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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464583525



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -101,7 +118,7 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
   }
 
   private Map<String, Boolean> getPartitionsHealth(String instance, ZNRecord partitionHealthRecord,
-      RESTConfig restConfig, Map<String, String> customPayLoads) {
+      RESTConfig restConfig, Map<String, String> customPayLoads, boolean requireFullRead) {

Review comment:
       I understand your concern. But it will be confusing to use one flag because they are completely in different purpose:
   1. Skip the zk access for reading partition health.
   2. Because of the contract we made for customized API, if no entry as body for "PARTITION", they will return all the partition health status. If they have it but partition list is empty, no result will be return. Here' when we only do partition health retrieving, we dont know what are the partitions on the instance. Also this function I would say is a separate function and logically not connected with "skipZKRead"
   
   I can add some comments in the function and rename the function. But I do not think make this name to be "onlyUseZkAsSource" is a good choice. There are no logic related to ZK for this private function.




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



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

[GitHub] [helix] dasahcc commented on pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on pull request #1197:
URL: https://github.com/apache/helix/pull/1197#issuecomment-669644256


   > One question: Do we want to change these public APIs? Maybe we can use some default value for the skipZKRead so we are not causing backward compatibility issues for existing usages?
   
   @alirezazamani Good question. But for the class itself, they are purely used for REST endpoint, as long as we keep the REST backward compatible, the class we treated as internal.
   
   Different from helix-core, helix-core will be careful since we provided as an library and especially the interfaces under API module should not be changed.


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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464205660



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -101,7 +118,7 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor dataAccessor) {
   }
 
   private Map<String, Boolean> getPartitionsHealth(String instance, ZNRecord partitionHealthRecord,
-      RESTConfig restConfig, Map<String, String> customPayLoads) {
+      RESTConfig restConfig, Map<String, String> customPayLoads, boolean requireFullRead) {

Review comment:
       I suggest we use something like `onlyUseZkAsSource` or something as the boolean flag, and use the same name all throughout since having `skipZkRead` and `requireFullRead` here and there could be confusing down the road.




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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464206720



##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestInstanceService.java
##########
@@ -140,6 +151,70 @@ public void testGetInstanceStoppableCheckConnectionRefused() throws IOException
     Assert.assertEquals(actual.getFailedChecks().get(0), expectedFailedCheck);
   }
 
+  @Test
+  public void testCustomPartitionCheckWithSkipZKRead() throws IOException {
+    // Let ZK result is health, but http request is unhealthy.
+    // We expect the check fail if we skipZKRead.
+
+    String testPartition = "PARTITION_0";
+    String slibilingInstance = "instance0.linkedin.com_1236";
+    String jsonContent = "{\n" + "   \"param1\": \"value1\",\n" + "\"param2\": \"value2\"\n" + "}";

Review comment:
       Why does this jsonContent need multiple string concatenations?




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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464574757



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
##########
@@ -172,7 +172,7 @@ public Response instancesOperations(@PathParam("clusterId") String clusterId,
         admin.enableInstance(clusterId, enableInstances, false);
         break;
       case stoppable:
-        return batchGetStoppableInstances(clusterId, node);
+        return batchGetStoppableInstances(clusterId, node, Boolean.valueOf(skipZKRead));

Review comment:
       parseBoolean can cause exception if that does not match. But the valueOf has a default value.




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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464205957



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
##########
@@ -218,7 +218,8 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node) thr
       ObjectNode failedStoppableInstances = result.putObject(
           InstancesAccessor.InstancesProperties.instance_not_stoppable_with_reasons.name());
       InstanceService instanceService =
-          new InstanceServiceImpl(new HelixDataAccessorWrapper((ZKHelixDataAccessor) getDataAccssor(clusterId)), getConfigAccessor());
+          new InstanceServiceImpl(new HelixDataAccessorWrapper((ZKHelixDataAccessor) getDataAccssor(clusterId)), getConfigAccessor(),
+              skipZKRead);

Review comment:
       Nit: format?




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



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

[GitHub] [helix] narendly commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464205844



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java
##########
@@ -144,7 +144,7 @@ public Response getAllInstances(@PathParam("clusterId") String clusterId,
 
   @POST
   public Response instancesOperations(@PathParam("clusterId") String clusterId,
-      @QueryParam("command") String command, String content) {
+      @QueryParam("skipZKRead") String skipZKRead, @QueryParam("command") String command, String content) {

Review comment:
       Suggest we consolidate the nomenclature for the boolean flag (as explained above).




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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464576604



##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/service/TestInstanceService.java
##########
@@ -140,6 +151,70 @@ public void testGetInstanceStoppableCheckConnectionRefused() throws IOException
     Assert.assertEquals(actual.getFailedChecks().get(0), expectedFailedCheck);
   }
 
+  @Test
+  public void testCustomPartitionCheckWithSkipZKRead() throws IOException {
+    // Let ZK result is health, but http request is unhealthy.
+    // We expect the check fail if we skipZKRead.
+
+    String testPartition = "PARTITION_0";
+    String slibilingInstance = "instance0.linkedin.com_1236";
+    String jsonContent = "{\n" + "   \"param1\": \"value1\",\n" + "\"param2\": \"value2\"\n" + "}";

Review comment:
       Was for viewing purpose for each entry set a line.




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



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

[GitHub] [helix] dasahcc commented on a change in pull request #1197: Support option to skip direct ZK read for health check API

Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1197:
URL: https://github.com/apache/helix/pull/1197#discussion_r464576311



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -61,19 +61,32 @@
   private final HelixDataAccessorWrapper _dataAccessor;
   private final ConfigAccessor _configAccessor;
   private final CustomRestClient _customRestClient;
+  private boolean _skipZKRead;

Review comment:
       Final requires constructor have redundant code. Let me try to refactor it.




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



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