You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@helix.apache.org by "zpinto (via GitHub)" <gi...@apache.org> on 2023/10/05 02:05:01 UTC

[PR] CUSTOM_INSTANCE_CHECK and(or) CUSTOM_PARTITION_CHECK optional for stoppable APIs [helix]

zpinto opened a new pull request, #2641:
URL: https://github.com/apache/helix/pull/2641

   Allow for callers to helix-rest stoppable APIs to skip running CUSTOM_INSTANCE_CHECK and(or) CUSTOM_PARTITION_CHECK, as they may not have the need to implement additional custom checks.
   
   ### Description
   Customers are required to implement custom stoppable checks to use helix-rest stoppable APIs. Making these skippable removes that requirement.
   
   ### Tests
   
   - [x] testGetInstanceStoppableCheckWhenCustomInstanceCheckAndCustomPartitionCheckDisabled
      - Verifies none of the custom checks cause stoppable to be false
   - [x] testGetInstanceStoppableCheckWhenCustomPartitionCheckDisabled
      - Verifies instance custom check causes stoppable to be false
   - [x] testGetInstanceStoppableCheckWhenCustomInstanceCheckDisabled
      - Verifies partition custom check causes stoppable to be false
   - [x] Update TestPerInstanceAccessor and TestInstanceAccessor to make sure they can parse skipHealthCheckCategories
   
   ```
   ➜  helix git:(custom_stoppable_checks_optional) ✗ mvn clean install -Dmaven.test.skip.exec=true && mvn test -o -Dtest=TestMaintenanceManagementService,TestInstancesAccessor,TestPerInstanceAccessor -pl=helix-rest
   
   ...
   
   [INFO] Tests run: 37, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 127.184 s - in TestSuite
   [INFO] 
   [INFO] Results:
   [INFO] 
   [INFO] Tests run: 37, Failures: 0, Errors: 0, Skipped: 0
   [INFO] 
   [INFO] 
   [INFO] --- jacoco:0.8.6:report (generate-code-coverage-report) @ helix-rest ---
   [INFO] Loading execution data file /Users/zapinto/Documents/git/zpinto/helix/helix-rest/target/jacoco.exec
   [INFO] Analyzed bundle 'Apache Helix :: Restful Interface' with 92 classes
   [INFO] ------------------------------------------------------------------------
   [INFO] BUILD SUCCESS
   [INFO] ------------------------------------------------------------------------
   [INFO] Total time:  02:19 min
   [INFO] Finished at: 2023-10-04T18:59:33-07:00
   [INFO] ------------------------------------------------------------------------
   ```
   
   ### Changes that Break Backward Compatibility (Optional)
   
   - Changes are backwards compatible
   
   ### 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"
   
   ### 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.

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

Re: [PR] CUSTOM_INSTANCE_CHECK and(or) CUSTOM_PARTITION_CHECK optional for stoppable APIs [helix]

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.

xyuanlu merged PR #2641:
URL: https://github.com/apache/helix/pull/2641


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

Re: [PR] CUSTOM_INSTANCE_CHECK and(or) CUSTOM_PARTITION_CHECK optional for stoppable APIs [helix]

Posted by "desaikomal (via GitHub)" <gi...@apache.org>.

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


##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -150,19 +149,34 @@ public Response getAllInstances(@PathParam("clusterId") String clusterId,
   @ResponseMetered(name = HttpConstants.WRITE_REQUEST)
   @Timed(name = HttpConstants.WRITE_REQUEST)
   @POST
-  public Response instancesOperations(
-      @PathParam("clusterId") String clusterId,
+  public Response instancesOperations(@PathParam("clusterId") String clusterId,
       @QueryParam("command") String command,
       @QueryParam("continueOnFailures") boolean continueOnFailures,
       @QueryParam("skipZKRead") boolean skipZKRead,
-      String content) {
+      @QueryParam("skipHealthCheckCategories") String skipHealthCheckCategories, String content) {

Review Comment:
   wondering if want to have one generic -skipAllHealthCheck ?



##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -81,43 +82,65 @@ public class MaintenanceManagementService {
   public static final String HELIX_CUSTOM_STOPPABLE_CHECK = "CustomInstanceStoppableCheck";
   public static final String OPERATION_CONFIG_SHARED_INPUT = "OperationConfigSharedInput";
 
+  public static final Set<StoppableCheck.Category> SKIPPABLE_HEALTH_CHECK_CATEGORIES =
+      ImmutableSet.of(StoppableCheck.Category.CUSTOM_INSTANCE_CHECK,
+          StoppableCheck.Category.CUSTOM_PARTITION_CHECK);
+
   private final ConfigAccessor _configAccessor;
   private final CustomRestClient _customRestClient;
   private final String _namespace;
   private final boolean _skipZKRead;
   private final HelixDataAccessorWrapper _dataAccessor;
   private final Set<String> _nonBlockingHealthChecks;
+  private final Set<StoppableCheck.Category> _skipHealthCheckCategories;
 
   public MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
       ConfigAccessor configAccessor, boolean skipZKRead, String namespace) {
-    this(dataAccessor, configAccessor, CustomRestClientFactory.get(), skipZKRead,
+    this(new HelixDataAccessorWrapper(dataAccessor, CustomRestClientFactory.get(), namespace),

Review Comment:
   can you please clarify why are we changing each method to pass wrapper where as the earlier implementation had just one implementation which converted HelixDataAccessor to HelixDataAccessorWrapper? 



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