You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "MarkGaox (via GitHub)" <gi...@apache.org> on 2023/11/01 16:54:46 UTC
[PR] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
MarkGaox opened a new pull request, #2687:
URL: https://github.com/apache/helix/pull/2687
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
#2655
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
- Add support to filter out evacuating nodes during stoppable checks:
- `siblingNodesActiveReplicaCheck`
- `perPartitionHealthCheck`
- Provision the users with the capability to whitelist a subset of stoppable checks so that Helix will only perform the given subset of all stoppable checks.
### Tests
- [X] The following tests are written for this issue:
` mvn test -Dtest=TestInstancesAccessor,TestMaintenanceManagementService,TestInstanceValidationUtilInRest,TestPerInstanceAccessor -pl helix-rest && mvn test -Dtest=TestInstanceValidationUtil -pl helix-core`
```
[INFO] Tests run: 46, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 64.604 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 46, 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/xiaxgao/IdeaProjects/helix_ps/helix-rest/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Restful Interface' with 95 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:07 min
[INFO] Finished at: 2023-10-31T23:44:24-07:00
[INFO] ------------------------------------------------------------------------
```
```
[INFO] Tests run: 25, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.037 s - in org.apache.helix.util.TestInstanceValidationUtil
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 25, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco:0.8.6:report (generate-code-coverage-report) @ helix-core ---
[INFO] Loading execution data file /Users/xiaxgao/IdeaProjects/helix_ps/helix-core/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Core' with 947 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 10.553 s
[INFO] Finished at: 2023-10-31T23:31:04-07:00
[INFO] ------------------------------------------------------------------------
```
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### 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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380908834
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -159,13 +159,10 @@ private MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
_customRestClient = customRestClient;
_skipZKRead = skipZKRead;
_nonBlockingHealthChecks =
- continueOnFailure ? Collections.singleton(ALL_HEALTH_CHECK_NONBLOCK)
- : Collections.emptySet();
+ nonBlockingHealthChecks == null ? Collections.emptySet() : nonBlockingHealthChecks;
_skipHealthCheckCategories =
- skipHealthCheckCategories == null ? Collections.emptySet()
- : skipHealthCheckCategories;
- _stoppableHealthCheckList =
- stoppableHealthCheckList == null ? Collections.emptyList()
+ skipHealthCheckCategories == null ? Collections.emptySet() : skipHealthCheckCategories;
Review Comment:
change it from `stoppableCheckList` to be `skipStoppableCheckList`. And by default, no checks will be skipped.
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1379074539
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -144,6 +148,26 @@ public MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
_namespace = namespace;
}
+ @VisibleForTesting
+ MaintenanceManagementService(MaintenanceManagementServiceBuilder builder) {
Review Comment:
This is not right way of using builder. Use builder we don't want them to create instance from constructor and it should be private constructor.
##########
helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java:
##########
@@ -239,6 +240,23 @@ public static boolean hasErrorPartitions(HelixDataAccessor dataAccessor, String
return false;
}
+ /**
+ * Checks if the specified instance is marked for an ongoing instance operation. Currently,
+ * this method only checks for evacuation.
+ *
+ * @param dataAccessor The accessor for retrieving Helix data properties.
+ * @param instanceName An instance to be evaluated.
+ * @return
+ */
+ public static boolean isOperationSetForInstance(HelixDataAccessor dataAccessor,
+ String instanceName) {
+ PropertyKey.Builder propertyKeyBuilder = dataAccessor.keyBuilder();
+ InstanceConfig instanceConfig =
+ dataAccessor.getProperty(propertyKeyBuilder.instanceConfig(instanceName));
+ return InstanceConstants.InstanceOperation.EVACUATE.name()
Review Comment:
Why only evacuate? How about disabled.
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -260,10 +271,44 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
}
}
+ if (node.get(InstancesProperties.stoppable_check_list.name()) != null) {
+ List<String> list = OBJECT_MAPPER.readValue(
+ node.get(InstancesProperties.stoppable_check_list.name()).toString(),
+ OBJECT_MAPPER.getTypeFactory().constructCollectionType(List.class, String.class));
+
+ if (ALL_STOPPABLE_CHECK_LIST.equals(list)) {
+ stoppableCheckList = HealthCheck.STOPPABLE_CHECK_LIST;
+ } else {
+ try {
+ stoppableCheckList =
+ list.stream().map(HealthCheck::valueOf).collect(Collectors.toList());
+ } catch (IllegalArgumentException e) {
+ String message =
+ "'stoppable_check_list' has invalid check names: " + list
+ + ". Supported checks: " + HealthCheck.STOPPABLE_CHECK_LIST;
+ _logger.error(message, e);
+ return badRequest(message);
+ }
+ }
+ } else {
+ // By default, if stoppable_check_list is unset, all checks are performed to maintain
+ // backward compatibility with existing clients.
+ stoppableCheckList = HealthCheck.STOPPABLE_CHECK_LIST;
+ }
Review Comment:
This restructure the logic, it is not very clean. We can discuss offline.
##########
helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java:
##########
@@ -451,9 +474,11 @@ public static boolean siblingNodesActiveReplicaCheck(HelixDataAccessor dataAcces
if (stateByInstanceMap.containsKey(instanceName)) {
int numHealthySiblings = 0;
for (Map.Entry<String, String> entry : stateByInstanceMap.entrySet()) {
- if (!entry.getKey().equals(instanceName) && (toBeStoppedInstances == null
- || !toBeStoppedInstances.contains(entry.getKey())) && !unhealthyStates.contains(
- entry.getValue())) {
+ String siblingInstanceName = entry.getKey();
+ if (!siblingInstanceName.equals(instanceName) && (toBeStoppedInstances == null
+ || !toBeStoppedInstances.contains(siblingInstanceName))
+ && !unhealthyStates.contains(entry.getValue()) && !isOperationSetForInstance(
+ dataAccessor, siblingInstanceName)) {
Review Comment:
This increase the dimension of checks. Why not just let the instances already in operation in toBeStoppedInstances? Semantically it makes sense as well.
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementServiceBuilder.java:
##########
@@ -0,0 +1,127 @@
+package org.apache.helix.rest.clusterMaintenanceService;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.rest.client.CustomRestClient;
+import org.apache.helix.rest.server.json.instance.StoppableCheck;
+
+
+public class MaintenanceManagementServiceBuilder {
+ private ConfigAccessor _configAccessor;
+ private boolean _skipZKRead;
+ private String _namespace;
+ private ZKHelixDataAccessor _dataAccessor;
+ private CustomRestClient _customRestClient;
+ private boolean _continueOnFailure;
+ private Set<StoppableCheck.Category> _skipHealthCheckCategories = Collections.emptySet();
+ private List<HealthCheck> _stoppableHealthCheckList = Collections.emptyList();
+
+ public ConfigAccessor getConfigAccessor() {
+ return _configAccessor;
+ }
+
+ public boolean isSkipZKRead() {
+ return _skipZKRead;
+ }
+
+ public String getNamespace() {
+ return _namespace;
+ }
+
+ public ZKHelixDataAccessor getDataAccessor() {
+ return _dataAccessor;
+ }
+
+ public CustomRestClient getCustomRestClient() {
+ return _customRestClient;
+ }
+
+ public boolean isContinueOnFailure() {
+ return _continueOnFailure;
+ }
+
+ public Set<StoppableCheck.Category> getSkipHealthCheckCategories() {
+ return _skipHealthCheckCategories;
+ }
+
+ public List<HealthCheck> getStoppableHealthCheckList() {
+ return _stoppableHealthCheckList;
+ }
+
+ public MaintenanceManagementServiceBuilder setConfigAccessor(ConfigAccessor configAccessor) {
+ _configAccessor = configAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipZKRead(boolean skipZKRead) {
+ _skipZKRead = skipZKRead;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setNamespace(String namespace) {
+ _namespace = namespace;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setDataAccessor(
+ ZKHelixDataAccessor dataAccessor) {
+ _dataAccessor = dataAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setCustomRestClient(
+ CustomRestClient customRestClient) {
+ _customRestClient = customRestClient;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setContinueOnFailure(boolean continueOnFailure) {
+ _continueOnFailure = continueOnFailure;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipHealthCheckCategories(
+ Set<StoppableCheck.Category> skipHealthCheckCategories) {
+ _skipHealthCheckCategories = skipHealthCheckCategories;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setStoppableHealthCheckList(
+ List<HealthCheck> stoppableHealthCheckList) {
+ _stoppableHealthCheckList = stoppableHealthCheckList;
+ return this;
+ }
+
+ public MaintenanceManagementService createMaintenanceManagementService() {
Review Comment:
Java convention we split this as build() method and validate method.
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementServiceBuilder.java:
##########
@@ -0,0 +1,127 @@
+package org.apache.helix.rest.clusterMaintenanceService;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.helix.ConfigAccessor;
+import org.apache.helix.manager.zk.ZKHelixDataAccessor;
+import org.apache.helix.rest.client.CustomRestClient;
+import org.apache.helix.rest.server.json.instance.StoppableCheck;
+
+
+public class MaintenanceManagementServiceBuilder {
+ private ConfigAccessor _configAccessor;
+ private boolean _skipZKRead;
+ private String _namespace;
+ private ZKHelixDataAccessor _dataAccessor;
+ private CustomRestClient _customRestClient;
+ private boolean _continueOnFailure;
+ private Set<StoppableCheck.Category> _skipHealthCheckCategories = Collections.emptySet();
+ private List<HealthCheck> _stoppableHealthCheckList = Collections.emptyList();
+
+ public ConfigAccessor getConfigAccessor() {
+ return _configAccessor;
+ }
+
+ public boolean isSkipZKRead() {
+ return _skipZKRead;
+ }
+
+ public String getNamespace() {
+ return _namespace;
+ }
+
+ public ZKHelixDataAccessor getDataAccessor() {
+ return _dataAccessor;
+ }
+
+ public CustomRestClient getCustomRestClient() {
+ return _customRestClient;
+ }
+
+ public boolean isContinueOnFailure() {
+ return _continueOnFailure;
+ }
+
+ public Set<StoppableCheck.Category> getSkipHealthCheckCategories() {
+ return _skipHealthCheckCategories;
+ }
+
+ public List<HealthCheck> getStoppableHealthCheckList() {
+ return _stoppableHealthCheckList;
+ }
+
+ public MaintenanceManagementServiceBuilder setConfigAccessor(ConfigAccessor configAccessor) {
+ _configAccessor = configAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipZKRead(boolean skipZKRead) {
+ _skipZKRead = skipZKRead;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setNamespace(String namespace) {
+ _namespace = namespace;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setDataAccessor(
+ ZKHelixDataAccessor dataAccessor) {
+ _dataAccessor = dataAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setCustomRestClient(
+ CustomRestClient customRestClient) {
+ _customRestClient = customRestClient;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setContinueOnFailure(boolean continueOnFailure) {
+ _continueOnFailure = continueOnFailure;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipHealthCheckCategories(
+ Set<StoppableCheck.Category> skipHealthCheckCategories) {
+ _skipHealthCheckCategories = skipHealthCheckCategories;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setStoppableHealthCheckList(
+ List<HealthCheck> stoppableHealthCheckList) {
+ _stoppableHealthCheckList = stoppableHealthCheckList;
+ return this;
+ }
+
+ public MaintenanceManagementService createMaintenanceManagementService() {
+ if (_configAccessor == null || _namespace == null || _dataAccessor == null
+ || _customRestClient == null) {
+ throw new IllegalArgumentException(
+ "One or more of following mandatory arguments are not set: '_configAccessor', "
+ + "'_namespace', '_dataAccessor', '_customRestClient'.");
+ }
+ return new MaintenanceManagementService(this);
+ }
+}
Review Comment:
Add a new line 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.
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380924110
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -59,17 +61,21 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.helix.rest.clusterMaintenanceService.MaintenanceManagementService.ALL_HEALTH_CHECK_NONBLOCK;
+
@ClusterAuth
@Path("/clusters/{clusterId}/instances")
public class InstancesAccessor extends AbstractHelixResource {
private final static Logger _logger = LoggerFactory.getLogger(InstancesAccessor.class);
+
public enum InstancesProperties {
instances,
online,
disabled,
selection_base,
zone_order,
to_be_stopped_instances,
+ stoppable_check_list,
Review Comment:
Change it from `stoppableCheckList` to be `skipStoppableCheckList` to preserve the overall consistency of the API. And by default, no checks will be skipped, which will maintain the compatibility.
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2687:
URL: https://github.com/apache/helix/pull/2687
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1379449091
##########
helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java:
##########
@@ -451,9 +474,11 @@ public static boolean siblingNodesActiveReplicaCheck(HelixDataAccessor dataAcces
if (stateByInstanceMap.containsKey(instanceName)) {
int numHealthySiblings = 0;
for (Map.Entry<String, String> entry : stateByInstanceMap.entrySet()) {
- if (!entry.getKey().equals(instanceName) && (toBeStoppedInstances == null
- || !toBeStoppedInstances.contains(entry.getKey())) && !unhealthyStates.contains(
- entry.getValue())) {
+ String siblingInstanceName = entry.getKey();
+ if (!siblingInstanceName.equals(instanceName) && (toBeStoppedInstances == null
+ || !toBeStoppedInstances.contains(siblingInstanceName))
+ && !unhealthyStates.contains(entry.getValue()) && !isOperationSetForInstance(
+ dataAccessor, siblingInstanceName)) {
Review Comment:
Sounds good. Will delegate the evacuating node filtering to `StoppablInstancesSelector`, in which it would go through all instances in the topology and add evacuating nodes to the `toBeStoppedInstances`
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -144,6 +148,26 @@ public MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
_namespace = namespace;
}
+ @VisibleForTesting
+ MaintenanceManagementService(MaintenanceManagementServiceBuilder builder) {
Review Comment:
Fixed
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -260,10 +271,44 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
}
}
+ if (node.get(InstancesProperties.stoppable_check_list.name()) != null) {
+ List<String> list = OBJECT_MAPPER.readValue(
+ node.get(InstancesProperties.stoppable_check_list.name()).toString(),
+ OBJECT_MAPPER.getTypeFactory().constructCollectionType(List.class, String.class));
+
+ if (ALL_STOPPABLE_CHECK_LIST.equals(list)) {
+ stoppableCheckList = HealthCheck.STOPPABLE_CHECK_LIST;
+ } else {
+ try {
+ stoppableCheckList =
+ list.stream().map(HealthCheck::valueOf).collect(Collectors.toList());
+ } catch (IllegalArgumentException e) {
+ String message =
+ "'stoppable_check_list' has invalid check names: " + list
+ + ". Supported checks: " + HealthCheck.STOPPABLE_CHECK_LIST;
+ _logger.error(message, e);
+ return badRequest(message);
+ }
+ }
+ } else {
+ // By default, if stoppable_check_list is unset, all checks are performed to maintain
+ // backward compatibility with existing clients.
+ stoppableCheckList = HealthCheck.STOPPABLE_CHECK_LIST;
+ }
Review Comment:
Logic is cleaned up as follow
- If the `STOPPABLE_CHECK_LIST` is not provided, Helix will perform **all Helix own stoppable checks** by default. And this will help with the backward compatibility since all existing users would expect Helix to perform all checks.
- If the `STOPPABLE_CHECK_LIST` is provided,
- If it's empty, Helix will skip all Helix own checks.
- If it's not empty, Helix will only perform the given checks and carry out the checks in the order they are listed.
One more thing to note. In the current code, the HELIX:INSTANCE_NOT_EXIST will always be performed because I think it doesn't make sense to mark instances as stoppable if they don't even exist.
Thanks for bringing this 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.
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380837921
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -59,17 +61,21 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.helix.rest.clusterMaintenanceService.MaintenanceManagementService.ALL_HEALTH_CHECK_NONBLOCK;
+
@ClusterAuth
@Path("/clusters/{clusterId}/instances")
public class InstancesAccessor extends AbstractHelixResource {
private final static Logger _logger = LoggerFactory.getLogger(InstancesAccessor.class);
+
public enum InstancesProperties {
instances,
online,
disabled,
selection_base,
zone_order,
to_be_stopped_instances,
+ stoppable_check_list,
Review Comment:
Call this stoppable_check_list_override is to make it more transparent to the user that passing a list for this will override the default behavior of using all checks. If unset, it will use the default.
##########
helix-rest/src/test/java/org/apache/helix/rest/server/AbstractTestClass.java:
##########
@@ -564,6 +564,8 @@ private void preSetupForParallelInstancesStoppableTest(String clusterName,
clusterConfig.setFaultZoneType("helixZoneId");
clusterConfig.setPersistIntermediateAssignment(true);
_configAccessor.setClusterConfig(clusterName, clusterConfig);
+ RESTConfig emptyRestConfig = new RESTConfig(clusterName);
Review Comment:
Let's not create fake RESTConfig that does not have the required fields.
If causes unexpected behavior on https://github.com/apache/helix/blob/master/helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java#L459C9-L459C27
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on PR #2687:
URL: https://github.com/apache/helix/pull/2687#issuecomment-1792933167
Thanks @junkaixue @xyuanlu @zpinto for review. This PR is approved by @junkaixue @zpinto.
Final commit message: Enhanced stoppable checks with evacuating nodes filtering and introduced blacklisting capabilities
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380966526
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -93,10 +93,10 @@ public class MaintenanceManagementService {
private final HelixDataAccessorWrapper _dataAccessor;
private final Set<String> _nonBlockingHealthChecks;
private final Set<StoppableCheck.Category> _skipHealthCheckCategories;
- // Set the default value of _stoppableHealthCheckList to be the list of all stoppable checks to
+ // Set the default value of _skipStoppableHealthCheckList to be an empty list to
// maintain the backward compatibility with users who don't use MaintenanceManagementServiceBuilder
// to create the MaintenanceManagementService object.
- private List<HealthCheck> _stoppableHealthCheckList = HealthCheck.STOPPABLE_CHECK_LIST;
+ private List<HealthCheck> _skipStoppableHealthCheckList = Collections.emptyList();
Review Comment:
Why set this now? We reset it in constructor anyway.
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1379093505
##########
helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java:
##########
@@ -239,6 +240,23 @@ public static boolean hasErrorPartitions(HelixDataAccessor dataAccessor, String
return false;
}
+ /**
+ * Checks if the specified instance is marked for an ongoing instance operation. Currently,
+ * this method only checks for evacuation.
+ *
+ * @param dataAccessor The accessor for retrieving Helix data properties.
+ * @param instanceName An instance to be evaluated.
+ * @return
+ */
+ public static boolean isOperationSetForInstance(HelixDataAccessor dataAccessor,
+ String instanceName) {
+ PropertyKey.Builder propertyKeyBuilder = dataAccessor.keyBuilder();
+ InstanceConfig instanceConfig =
+ dataAccessor.getProperty(propertyKeyBuilder.instanceConfig(instanceName));
+ return InstanceConstants.InstanceOperation.EVACUATE.name()
Review Comment:
There is another check `INSTANCE_NOT_ENABLED` that does this `InstanceValidationUtil.isEnabled(_dataAccessor, instanceName));`
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380839712
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -159,13 +159,10 @@ private MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
_customRestClient = customRestClient;
_skipZKRead = skipZKRead;
_nonBlockingHealthChecks =
- continueOnFailure ? Collections.singleton(ALL_HEALTH_CHECK_NONBLOCK)
- : Collections.emptySet();
+ nonBlockingHealthChecks == null ? Collections.emptySet() : nonBlockingHealthChecks;
_skipHealthCheckCategories =
- skipHealthCheckCategories == null ? Collections.emptySet()
- : skipHealthCheckCategories;
- _stoppableHealthCheckList =
- stoppableHealthCheckList == null ? Collections.emptyList()
+ skipHealthCheckCategories == null ? Collections.emptySet() : skipHealthCheckCategories;
Review Comment:
Please consider to have homogeneous input. For example, we have flack list for skip customized check and whit list for helix health check. It is a bit confusing.
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1381142849
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -93,10 +93,10 @@ public class MaintenanceManagementService {
private final HelixDataAccessorWrapper _dataAccessor;
private final Set<String> _nonBlockingHealthChecks;
private final Set<StoppableCheck.Category> _skipHealthCheckCategories;
- // Set the default value of _stoppableHealthCheckList to be the list of all stoppable checks to
+ // Set the default value of _skipStoppableHealthCheckList to be an empty list to
// maintain the backward compatibility with users who don't use MaintenanceManagementServiceBuilder
// to create the MaintenanceManagementService object.
- private List<HealthCheck> _stoppableHealthCheckList = HealthCheck.STOPPABLE_CHECK_LIST;
+ private List<HealthCheck> _skipStoppableHealthCheckList = Collections.emptyList();
Review Comment:
This is for users who didn't use `MaintainenceServiceBuilder` to `MaintainenceService`. Since I didn't modify the existing constructors, they can't set `_skipStoppableHealthCheckList ` in their code, which means `_skipStoppableHealthCheckList` has have 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.
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380908834
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -159,13 +159,10 @@ private MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
_customRestClient = customRestClient;
_skipZKRead = skipZKRead;
_nonBlockingHealthChecks =
- continueOnFailure ? Collections.singleton(ALL_HEALTH_CHECK_NONBLOCK)
- : Collections.emptySet();
+ nonBlockingHealthChecks == null ? Collections.emptySet() : nonBlockingHealthChecks;
_skipHealthCheckCategories =
- skipHealthCheckCategories == null ? Collections.emptySet()
- : skipHealthCheckCategories;
- _stoppableHealthCheckList =
- stoppableHealthCheckList == null ? Collections.emptyList()
+ skipHealthCheckCategories == null ? Collections.emptySet() : skipHealthCheckCategories;
Review Comment:
change it from `stoppableCheckList` to `skipStoppableCheckList`. And by default, no checks will be skipped.
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380628022
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -771,4 +797,86 @@ protected Map<String, Boolean> getInstanceHealthStatus(String clusterId, String
return healthStatus;
}
+
+ public static class MaintenanceManagementServiceBuilder {
+ private ConfigAccessor _configAccessor;
+ private boolean _skipZKRead;
+ private String _namespace;
+ private ZKHelixDataAccessor _dataAccessor;
+ private CustomRestClient _customRestClient;
+ private boolean _continueOnFailure;
+ private Set<StoppableCheck.Category> _skipHealthCheckCategories = Collections.emptySet();
+ private List<HealthCheck> _stoppableHealthCheckList = Collections.emptyList();
+
+ public MaintenanceManagementServiceBuilder setConfigAccessor(ConfigAccessor configAccessor) {
+ _configAccessor = configAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipZKRead(boolean skipZKRead) {
+ _skipZKRead = skipZKRead;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setNamespace(String namespace) {
+ _namespace = namespace;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setDataAccessor(
+ ZKHelixDataAccessor dataAccessor) {
+ _dataAccessor = dataAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setCustomRestClient(
+ CustomRestClient customRestClient) {
+ _customRestClient = customRestClient;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setContinueOnFailure(boolean continueOnFailure) {
+ _continueOnFailure = continueOnFailure;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipHealthCheckCategories(
+ Set<StoppableCheck.Category> skipHealthCheckCategories) {
+ _skipHealthCheckCategories = skipHealthCheckCategories;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setStoppableHealthCheckList(
+ List<HealthCheck> stoppableHealthCheckList) {
+ _stoppableHealthCheckList = stoppableHealthCheckList;
Review Comment:
I think a better logic would be
* If the stoppableCheckList is given, Helix should perform the given checks.
* If the stoppableCheckList == null, Helix will perform all existing Helix own checks. This approach preserves backward compatibility, ensuring that existing users receive the full suite of checks they anticipate by default.
In this way, users don't need to pass [ALL] to indicate they want to all stoppable checks.
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1380445768
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -144,6 +148,28 @@ public MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
_namespace = namespace;
}
+ private MaintenanceManagementService(ZKHelixDataAccessor dataAccessor,
+ ConfigAccessor configAccessor, CustomRestClient customRestClient, boolean skipZKRead,
+ boolean continueOnFailure, Set<StoppableCheck.Category> skipHealthCheckCategories,
+ List<HealthCheck> stoppableHealthCheckList, String namespace) {
+ _dataAccessor =
+ new HelixDataAccessorWrapper(dataAccessor, customRestClient,
+ namespace);
+ _configAccessor = configAccessor;
+ _customRestClient = customRestClient;
+ _skipZKRead = skipZKRead;
+ _nonBlockingHealthChecks =
+ continueOnFailure ? Collections.singleton(ALL_HEALTH_CHECK_NONBLOCK)
Review Comment:
Why `continueOnFailure ` matters here?
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -771,4 +797,86 @@ protected Map<String, Boolean> getInstanceHealthStatus(String clusterId, String
return healthStatus;
}
+
+ public static class MaintenanceManagementServiceBuilder {
+ private ConfigAccessor _configAccessor;
+ private boolean _skipZKRead;
+ private String _namespace;
+ private ZKHelixDataAccessor _dataAccessor;
+ private CustomRestClient _customRestClient;
+ private boolean _continueOnFailure;
+ private Set<StoppableCheck.Category> _skipHealthCheckCategories = Collections.emptySet();
+ private List<HealthCheck> _stoppableHealthCheckList = Collections.emptyList();
+
+ public MaintenanceManagementServiceBuilder setConfigAccessor(ConfigAccessor configAccessor) {
+ _configAccessor = configAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipZKRead(boolean skipZKRead) {
+ _skipZKRead = skipZKRead;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setNamespace(String namespace) {
+ _namespace = namespace;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setDataAccessor(
+ ZKHelixDataAccessor dataAccessor) {
+ _dataAccessor = dataAccessor;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setCustomRestClient(
+ CustomRestClient customRestClient) {
+ _customRestClient = customRestClient;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setContinueOnFailure(boolean continueOnFailure) {
+ _continueOnFailure = continueOnFailure;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setSkipHealthCheckCategories(
+ Set<StoppableCheck.Category> skipHealthCheckCategories) {
+ _skipHealthCheckCategories = skipHealthCheckCategories;
+ return this;
+ }
+
+ public MaintenanceManagementServiceBuilder setStoppableHealthCheckList(
+ List<HealthCheck> stoppableHealthCheckList) {
+ _stoppableHealthCheckList = stoppableHealthCheckList;
Review Comment:
When user pass [ALL], we could consolidate a list with all checks.
--
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2687:
URL: https://github.com/apache/helix/pull/2687#discussion_r1381142849
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java:
##########
@@ -93,10 +93,10 @@ public class MaintenanceManagementService {
private final HelixDataAccessorWrapper _dataAccessor;
private final Set<String> _nonBlockingHealthChecks;
private final Set<StoppableCheck.Category> _skipHealthCheckCategories;
- // Set the default value of _stoppableHealthCheckList to be the list of all stoppable checks to
+ // Set the default value of _skipStoppableHealthCheckList to be an empty list to
// maintain the backward compatibility with users who don't use MaintenanceManagementServiceBuilder
// to create the MaintenanceManagementService object.
- private List<HealthCheck> _stoppableHealthCheckList = HealthCheck.STOPPABLE_CHECK_LIST;
+ private List<HealthCheck> _skipStoppableHealthCheckList = Collections.emptyList();
Review Comment:
This is for users that didn't use `MaintainenceServiceBuilder` to `MaintainenceService`. Since I didn't modify the existing constructors, they can't set `_skipStoppableHealthCheckList ` in their code, which means `_skipStoppableHealthCheckList` has have 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.
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] Implement node evacuation filtering during checks and add whitelist for stoppable checks [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on PR #2687:
URL: https://github.com/apache/helix/pull/2687#issuecomment-1793260683
Thanks @xyuanlu @zpinto @junkaixue for reviewing this PR. It's approved by @zpinto @junkaixue.
Final commit message:
Enhanced stoppable checks with node evacuation filtering and introduced blacklisting capabilities
--
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