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 2024/01/11 21:30:32 UTC
[PR] Implement 'StoppableCheck' Feature to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
MarkGaox opened a new pull request, #2736:
URL: https://github.com/apache/helix/pull/2736
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
- #2655
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
- This PR adds a new option in StoppableCheck API to allow users to choose whether allow the stoppable instances to exceed the maximum of offline instances allowed. If `notExceedMaxOfflineInstances` sets to true, the max Stoppable instances won't surpass the limit on the cluster configuration, which ensures the cluster won't enter the maintenance mode and do the emergency rebalance.
(Write a concise description including what, why, how)
### 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`
(List the names of added unit/integration tests)
- 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.)
```
[INFO] Tests run: 49, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 74.918 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 49, 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:18 min
[INFO] Finished at: 2024-01-11T13:28:56-08:00
[INFO] ------------------------------------------------------------------------
```
```
[INFO] Tests run: 24, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.94 s - in org.apache.helix.util.TestInstanceValidationUtil
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 24, 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: 11.892 s
[INFO] Finished at: 2024-01-11T12:58:10-08:00
[INFO] ------------------------------------------------------------------------
```
### 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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1465493205
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(EXCEED_MAX_OFFLINE_INSTANCES));
+ failedHelixOwnChecks = true;
+ }
+
+ if (!stoppableCheck.isStoppable()) {
+ for (String failedReason : stoppableCheck.getFailedChecks()) {
+ // HELIX_OWN_CHECK can always be added to the failedReasonsNode.
+ if (failedReason.startsWith(StoppableCheck.Category.HELIX_OWN_CHECK.getPrefix())) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
+ failedHelixOwnChecks = true;
+ continue;
+ }
+ // CUSTOM_INSTANCE_CHECK and CUSTOM_PARTITION_CHECK can only be added to the failedReasonsNode
+ // if continueOnFailure is true and there is no failed Helix_OWN_CHECKS.
+ if (_continueOnFailure && !failedHelixOwnChecks) {
Review Comment:
Say our maxAllowedOffline = 3, and there are 5 instances. The problem is when we do batchGetStoppable for these 5 instances, we would have to process all 5 in parallel. However, if we process them in parallel, then there is no easy way for the individual instance to know the stop of itself will violate the maxAllowedOffline = 3 constraint unless we put a lock on everything.
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1465877633
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -302,16 +305,36 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
ClusterService clusterService =
new ClusterServiceImpl(getDataAccssor(clusterId), getConfigAccessor());
ClusterTopology clusterTopology = clusterService.getClusterTopology(clusterId);
- StoppableInstancesSelector stoppableInstancesSelector =
+ StoppableInstancesSelector.StoppableInstancesSelectorBuilder builder =
new StoppableInstancesSelector.StoppableInstancesSelectorBuilder()
.setClusterId(clusterId)
.setOrderOfZone(orderOfZone)
.setCustomizedInput(customizedInput)
.setMaintenanceService(maintenanceService)
.setClusterTopology(clusterTopology)
.setDataAccessor((ZKHelixDataAccessor) getDataAccssor(clusterId))
- .build();
+ .setContinueOnFailure(continueOnFailures);
+
+ if (notExceedingMaxOfflineInstances) {
+ ClusterConfig clusterConfig = getConfigAccessor().getClusterConfig(clusterId);
+ if (clusterConfig == null) {
+ String message =
+ "Invalid cluster name: " + clusterId + ". Cluster config does not exist.";
+ _logger.error(message);
+ return badRequest(message);
+ }
+ // If maxOfflineInstancesAllowed is not set, it means there is no limit on the number of offline instances.
+ // Therefore, builder sets the maxOfflineInstancesAllowed to the default value, Integer.MAX_VALUE.
+ if (clusterConfig.getMaxOfflineInstancesAllowed() != -1) {
+ builder.setMaxAdditionalOfflineInstances(clusterConfig.getMaxOfflineInstancesAllowed());
Review Comment:
Can we keep -1 and have special handling when MaxOfflineInstances <0?
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1465518277
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(EXCEED_MAX_OFFLINE_INSTANCES));
+ failedHelixOwnChecks = true;
+ }
+
+ if (!stoppableCheck.isStoppable()) {
+ for (String failedReason : stoppableCheck.getFailedChecks()) {
+ // HELIX_OWN_CHECK can always be added to the failedReasonsNode.
+ if (failedReason.startsWith(StoppableCheck.Category.HELIX_OWN_CHECK.getPrefix())) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
+ failedHelixOwnChecks = true;
+ continue;
+ }
+ // CUSTOM_INSTANCE_CHECK and CUSTOM_PARTITION_CHECK can only be added to the failedReasonsNode
+ // if continueOnFailure is true and there is no failed Helix_OWN_CHECKS.
+ if (_continueOnFailure && !failedHelixOwnChecks) {
Review Comment:
My 2cents: first comes correctness and then comes performance optimization. i am not completely plugged into your work, but correctness is very important to focus on.
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2736:
URL: https://github.com/apache/helix/pull/2736
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1470552045
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
Review Comment:
Sorry I still don't understand. If allowedOfflineInstances == 0, it means any additional stoppable instances are not stoppable. So the current instance should have a failed reason `EXCEED_MAX_OFFLINE_INSTANCES`, isn't 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.
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1464373219
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
Review Comment:
==0 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.
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1464384494
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(EXCEED_MAX_OFFLINE_INSTANCES));
+ failedHelixOwnChecks = true;
+ }
+
+ if (!stoppableCheck.isStoppable()) {
+ for (String failedReason : stoppableCheck.getFailedChecks()) {
+ // HELIX_OWN_CHECK can always be added to the failedReasonsNode.
+ if (failedReason.startsWith(StoppableCheck.Category.HELIX_OWN_CHECK.getPrefix())) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
+ failedHelixOwnChecks = true;
+ continue;
+ }
+ // CUSTOM_INSTANCE_CHECK and CUSTOM_PARTITION_CHECK can only be added to the failedReasonsNode
+ // if continueOnFailure is true and there is no failed Helix_OWN_CHECKS.
+ if (_continueOnFailure && !failedHelixOwnChecks) {
Review Comment:
I feel like we should skip check when continueOnFailure is false as a perf improvement. Instead of filter it when consolidate result..?
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1468994177
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -302,16 +305,36 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
ClusterService clusterService =
new ClusterServiceImpl(getDataAccssor(clusterId), getConfigAccessor());
ClusterTopology clusterTopology = clusterService.getClusterTopology(clusterId);
- StoppableInstancesSelector stoppableInstancesSelector =
+ StoppableInstancesSelector.StoppableInstancesSelectorBuilder builder =
new StoppableInstancesSelector.StoppableInstancesSelectorBuilder()
.setClusterId(clusterId)
.setOrderOfZone(orderOfZone)
.setCustomizedInput(customizedInput)
.setMaintenanceService(maintenanceService)
.setClusterTopology(clusterTopology)
.setDataAccessor((ZKHelixDataAccessor) getDataAccssor(clusterId))
- .build();
+ .setContinueOnFailure(continueOnFailures);
+
+ if (notExceedingMaxOfflineInstances) {
+ ClusterConfig clusterConfig = getConfigAccessor().getClusterConfig(clusterId);
+ if (clusterConfig == null) {
+ String message =
+ "Invalid cluster name: " + clusterId + ". Cluster config does not exist.";
+ _logger.error(message);
+ return badRequest(message);
+ }
+ // If maxOfflineInstancesAllowed is not set, it means there is no limit on the number of offline instances.
+ // Therefore, builder sets the maxOfflineInstancesAllowed to the default value, Integer.MAX_VALUE.
+ if (clusterConfig.getMaxOfflineInstancesAllowed() != -1) {
+ builder.setMaxAdditionalOfflineInstances(clusterConfig.getMaxOfflineInstancesAllowed());
+ }
+ }
+
+ StoppableInstancesSelector stoppableInstancesSelector = builder.build();
stoppableInstancesSelector.calculateOrderOfZone(instances, random);
+ Set<String> invalidInstances = new HashSet<>(toBeStoppedInstances);
Review Comment:
Good catch!
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
Review Comment:
It won't work if the size of `toBeStoppedInstances` > size of `maxAdditionalOfflineInstance`
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
Review Comment:
Good catch. Will count the number of offline instances in current cluster, and set `maxAdditionalOfflineInstance` = `maxAllowedOfflineInstance` - `currentOfflineInstance`
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -302,16 +305,36 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
ClusterService clusterService =
new ClusterServiceImpl(getDataAccssor(clusterId), getConfigAccessor());
ClusterTopology clusterTopology = clusterService.getClusterTopology(clusterId);
- StoppableInstancesSelector stoppableInstancesSelector =
+ StoppableInstancesSelector.StoppableInstancesSelectorBuilder builder =
new StoppableInstancesSelector.StoppableInstancesSelectorBuilder()
.setClusterId(clusterId)
.setOrderOfZone(orderOfZone)
.setCustomizedInput(customizedInput)
.setMaintenanceService(maintenanceService)
.setClusterTopology(clusterTopology)
.setDataAccessor((ZKHelixDataAccessor) getDataAccssor(clusterId))
- .build();
+ .setContinueOnFailure(continueOnFailures);
+
+ if (notExceedingMaxOfflineInstances) {
+ ClusterConfig clusterConfig = getConfigAccessor().getClusterConfig(clusterId);
+ if (clusterConfig == null) {
+ String message =
+ "Invalid cluster name: " + clusterId + ". Cluster config does not exist.";
+ _logger.error(message);
+ return badRequest(message);
+ }
+ // If maxOfflineInstancesAllowed is not set, it means there is no limit on the number of offline instances.
+ // Therefore, builder sets the maxOfflineInstancesAllowed to the default value, Integer.MAX_VALUE.
+ if (clusterConfig.getMaxOfflineInstancesAllowed() != -1) {
+ builder.setMaxAdditionalOfflineInstances(clusterConfig.getMaxOfflineInstancesAllowed());
Review Comment:
I think a even more reasonable solution is to not allow user do stoppableCheck if they didn't provide `maxOfflineInstancesAllowed` in their cluster config. What do you think?
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1468993962
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
Review Comment:
Since `allowedOfflineCount` is calculated by `maxAdditionalOfflineInstance` - `toBeStoppedInstances`, It won't work if `toBeStoppedInstances` > `maxAdditionalOfflineInstance`
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1465508076
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(EXCEED_MAX_OFFLINE_INSTANCES));
+ failedHelixOwnChecks = true;
+ }
+
+ if (!stoppableCheck.isStoppable()) {
+ for (String failedReason : stoppableCheck.getFailedChecks()) {
+ // HELIX_OWN_CHECK can always be added to the failedReasonsNode.
+ if (failedReason.startsWith(StoppableCheck.Category.HELIX_OWN_CHECK.getPrefix())) {
+ failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
+ failedHelixOwnChecks = true;
+ continue;
+ }
+ // CUSTOM_INSTANCE_CHECK and CUSTOM_PARTITION_CHECK can only be added to the failedReasonsNode
+ // if continueOnFailure is true and there is no failed Helix_OWN_CHECKS.
+ if (_continueOnFailure && !failedHelixOwnChecks) {
Review Comment:
Another way to handle this is to process instances by the amount of maxAllowedOffline. Say our maxAllowedOffline = 3, and there are 10 instances in the same zone. In the first iteration, we process instance1-3. If the cumulative stoppable instances count doesn't exceed maxAllowedOffline, we do the next iteration of instances4-6 and so on. But I'm worried about the performance in this design because now our API can only batchProcess `maxAllowedOffline` number of instances in parallel. If the instanceList is super long, our check could take many iteration to be finished.
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1465869252
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -302,16 +305,36 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
ClusterService clusterService =
new ClusterServiceImpl(getDataAccssor(clusterId), getConfigAccessor());
ClusterTopology clusterTopology = clusterService.getClusterTopology(clusterId);
- StoppableInstancesSelector stoppableInstancesSelector =
+ StoppableInstancesSelector.StoppableInstancesSelectorBuilder builder =
new StoppableInstancesSelector.StoppableInstancesSelectorBuilder()
.setClusterId(clusterId)
.setOrderOfZone(orderOfZone)
.setCustomizedInput(customizedInput)
.setMaintenanceService(maintenanceService)
.setClusterTopology(clusterTopology)
.setDataAccessor((ZKHelixDataAccessor) getDataAccssor(clusterId))
- .build();
+ .setContinueOnFailure(continueOnFailures);
+
+ if (notExceedingMaxOfflineInstances) {
+ ClusterConfig clusterConfig = getConfigAccessor().getClusterConfig(clusterId);
+ if (clusterConfig == null) {
+ String message =
+ "Invalid cluster name: " + clusterId + ". Cluster config does not exist.";
+ _logger.error(message);
+ return badRequest(message);
+ }
+ // If maxOfflineInstancesAllowed is not set, it means there is no limit on the number of offline instances.
+ // Therefore, builder sets the maxOfflineInstancesAllowed to the default value, Integer.MAX_VALUE.
+ if (clusterConfig.getMaxOfflineInstancesAllowed() != -1) {
+ builder.setMaxAdditionalOfflineInstances(clusterConfig.getMaxOfflineInstancesAllowed());
+ }
+ }
+
+ StoppableInstancesSelector stoppableInstancesSelector = builder.build();
stoppableInstancesSelector.calculateOrderOfZone(instances, random);
+ Set<String> invalidInstances = new HashSet<>(toBeStoppedInstances);
Review Comment:
nit: use stream and filter our instances that are not in clusterTopology.getAllInstances()
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1470537015
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
+ continue;
+ }
+ ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
+ boolean failedHelixOwnChecks = false;
+ if (allowedOfflineInstances <= 0) {
Review Comment:
Sorry I mean, should we just do allowedOfflineInstances < 0?
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1466752938
##########
helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/StoppableInstancesSelector.java:
##########
@@ -129,32 +136,53 @@ public ObjectNode getStoppableInstancesCrossZones(List<String> instances,
if (instanceSet.isEmpty()) {
continue;
}
- populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet, stoppableInstances,
- failedStoppableInstances);
+ populateStoppableInstances(new ArrayList<>(instanceSet), toBeStoppedInstancesSet,
+ stoppableInstances, failedStoppableInstances,
+ _maxAdditionalOfflineInstances - toBeStoppedInstancesSet.size());
}
processNonexistentInstances(instances, failedStoppableInstances);
return result;
}
private void populateStoppableInstances(List<String> instances, Set<String> toBeStoppedInstances,
- ArrayNode stoppableInstances, ObjectNode failedStoppableInstances) throws IOException {
+ ArrayNode stoppableInstances, ObjectNode failedStoppableInstances,
+ int allowedOfflineInstances) throws IOException {
Map<String, StoppableCheck> instancesStoppableChecks =
_maintenanceService.batchGetInstancesStoppableChecks(_clusterId, instances,
_customizedInput, toBeStoppedInstances);
for (Map.Entry<String, StoppableCheck> instanceStoppableCheck : instancesStoppableChecks.entrySet()) {
String instance = instanceStoppableCheck.getKey();
StoppableCheck stoppableCheck = instanceStoppableCheck.getValue();
- if (!stoppableCheck.isStoppable()) {
- ArrayNode failedReasonsNode = failedStoppableInstances.putArray(instance);
- for (String failedReason : stoppableCheck.getFailedChecks()) {
- failedReasonsNode.add(JsonNodeFactory.instance.textNode(failedReason));
- }
- } else {
+ if (stoppableCheck.isStoppable() && allowedOfflineInstances > 0) {
stoppableInstances.add(instance);
// Update the toBeStoppedInstances set with the currently identified stoppable instance.
// This ensures that subsequent checks in other zones are aware of this instance's stoppable status.
toBeStoppedInstances.add(instance);
+ allowedOfflineInstances--;
Review Comment:
We also need to check how many instances are already offline in the cluster.
--
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 'StoppableCheck' Flag to Ensure Maintenance Mode is Avoided After Stoppable Instances are Shutdown [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2736:
URL: https://github.com/apache/helix/pull/2736#discussion_r1470342814
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/InstancesAccessor.java:
##########
@@ -302,16 +305,50 @@ private Response batchGetStoppableInstances(String clusterId, JsonNode node, boo
ClusterService clusterService =
new ClusterServiceImpl(getDataAccssor(clusterId), getConfigAccessor());
ClusterTopology clusterTopology = clusterService.getClusterTopology(clusterId);
- StoppableInstancesSelector stoppableInstancesSelector =
+ StoppableInstancesSelector.StoppableInstancesSelectorBuilder builder =
new StoppableInstancesSelector.StoppableInstancesSelectorBuilder()
.setClusterId(clusterId)
.setOrderOfZone(orderOfZone)
.setCustomizedInput(customizedInput)
.setMaintenanceService(maintenanceService)
.setClusterTopology(clusterTopology)
.setDataAccessor((ZKHelixDataAccessor) getDataAccssor(clusterId))
- .build();
+ .setContinueOnFailure(continueOnFailures);
+
+ Set<String> currentOfflineInstances = Collections.emptySet();
+ if (notExceedingMaxOfflineInstances) {
+ // If the clusterConfig or maxOfflineInstancesAllowed is not set, this is an invalid request.
+ ClusterConfig clusterConfig = getConfigAccessor().getClusterConfig(clusterId);
+ if (clusterConfig == null) {
+ String message =
+ "Invalid cluster name: " + clusterId + ". Cluster config does not exist.";
+ _logger.error(message);
+ return badRequest(message);
+ }
+ int maxOfflineAllowed = clusterConfig.getMaxOfflineInstancesAllowed();
+ if (maxOfflineAllowed == -1) {
+ String message =
+ "Invalid cluster config: " + clusterId + ". maxOfflineInstancesAllowed is not set.";
+ _logger.error(message);
+ return badRequest(message);
+ }
+
+ HelixDataAccessor dataAccessor = getDataAccssor(clusterId);
+ List<String> liveInstances =
+ dataAccessor.getChildNames(dataAccessor.keyBuilder().liveInstances());
+ currentOfflineInstances = clusterTopology.getAllInstances().stream()
+ .filter(instance -> !liveInstances.contains(instance)).collect(Collectors.toSet());
+ maxOfflineAllowed = Math.max(0, maxOfflineAllowed - currentOfflineInstances.size());
+ builder.setMaxAdditionalOfflineInstances(maxOfflineAllowed);
+ }
+
+ StoppableInstancesSelector stoppableInstancesSelector = builder.build();
stoppableInstancesSelector.calculateOrderOfZone(instances, random);
+ Set<String> finalCurrentOfflineInstances = currentOfflineInstances;
Review Comment:
Why we want to create a new list?
--
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