You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "GrantPSpencer (via GitHub)" <gi...@apache.org> on 2024/01/24 20:14:17 UTC
[PR] Allow partitionAssignment API in maintenance mode [helix]
GrantPSpencer opened a new pull request, #2742:
URL: https://github.com/apache/helix/pull/2742
### Issues
Currently we do not return a partitionAssignment result if the cluster is in maintenance mode.
Allowing this would enable customers to make a series of changes to their cluster while in maintenance and then query the partitionAssignment one single time to get the expected assignments, without each change to the cluster triggering a rebalance.
### Description
Allow partitionAssignment API to calculate an assignment while the cluster is in maintenance mode. This also includes a minor change to WAGED rebalancer in BestPossibleCalcStage, as `computeResourceBestPossibleStateWithWagedRebalancer` currently returns an empty map if the cluster is in maintenance mode.
### Tests
- [x] The following tests are written for this issue:
testComputePartitionAssignmentMaintenanceMode
### Changes that Break Backward Compatibility (Optional)
- [x] My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
N/A
### 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] Allow partitionAssignment API in maintenance mode [helix]
Posted by "GrantPSpencer (via GitHub)" <gi...@apache.org>.
GrantPSpencer commented on code in PR #2742:
URL: https://github.com/apache/helix/pull/2742#discussion_r1465660048
##########
helix-rest/src/test/java/org/apache/helix/rest/server/TestPartitionAssignmentAPI.java:
##########
@@ -353,4 +355,114 @@ private void createWagedResource(String db, int numPartition, int minActiveRepli
Assert.assertTrue(_clusterVerifier.verifyByPolling());
}
+
+ private void createCrushedResource(String db, int numPartition, int minActiveReplica, long delay) {
+ _gSetupTool.addResourceToCluster(CLUSTER_NAME, db, numPartition, "LeaderStandby",
+ IdealState.RebalanceMode.FULL_AUTO + "", null);
+ _resources.add(db);
+
+ IdealState idealState =
+ _gSetupTool.getClusterManagementTool().getResourceIdealState(CLUSTER_NAME, db);
+ idealState.setMinActiveReplicas(minActiveReplica);
+ idealState.setDelayRebalanceEnabled(true);
+ idealState.setRebalanceDelay(delay);
+ idealState.setRebalancerClassName(DelayedAutoRebalancer.class.getName());
+ idealState.setRebalanceStrategy(CrushEdRebalanceStrategy.class.getName());
+ _gSetupTool.getClusterManagementTool().setResourceIdealState(CLUSTER_NAME, db, idealState);
+
+ ResourceConfig resourceConfig = new ResourceConfig(db);
+ _configAccessor.setResourceConfig(CLUSTER_NAME, db, resourceConfig);
+ _gSetupTool.rebalanceResource(CLUSTER_NAME, db, REPLICAS);
+
+ Assert.assertTrue(_clusterVerifier.verifyByPolling());
+ }
+ @Test
Review Comment:
good eye, thanks
--
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] Allow partitionAssignment API in maintenance mode [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2742:
URL: https://github.com/apache/helix/pull/2742
--
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] Allow partitionAssignment API in maintenance mode [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on code in PR #2742:
URL: https://github.com/apache/helix/pull/2742#discussion_r1465631483
##########
helix-rest/src/test/java/org/apache/helix/rest/server/TestPartitionAssignmentAPI.java:
##########
@@ -353,4 +355,114 @@ private void createWagedResource(String db, int numPartition, int minActiveRepli
Assert.assertTrue(_clusterVerifier.verifyByPolling());
}
+
+ private void createCrushedResource(String db, int numPartition, int minActiveReplica, long delay) {
+ _gSetupTool.addResourceToCluster(CLUSTER_NAME, db, numPartition, "LeaderStandby",
+ IdealState.RebalanceMode.FULL_AUTO + "", null);
+ _resources.add(db);
+
+ IdealState idealState =
+ _gSetupTool.getClusterManagementTool().getResourceIdealState(CLUSTER_NAME, db);
+ idealState.setMinActiveReplicas(minActiveReplica);
+ idealState.setDelayRebalanceEnabled(true);
+ idealState.setRebalanceDelay(delay);
+ idealState.setRebalancerClassName(DelayedAutoRebalancer.class.getName());
+ idealState.setRebalanceStrategy(CrushEdRebalanceStrategy.class.getName());
+ _gSetupTool.getClusterManagementTool().setResourceIdealState(CLUSTER_NAME, db, idealState);
+
+ ResourceConfig resourceConfig = new ResourceConfig(db);
+ _configAccessor.setResourceConfig(CLUSTER_NAME, db, resourceConfig);
+ _gSetupTool.rebalanceResource(CLUSTER_NAME, db, REPLICAS);
+
+ Assert.assertTrue(_clusterVerifier.verifyByPolling());
+ }
+ @Test
+ private void testComputePartitionAssignmentMaintenanceMode() throws Exception {
+
+ // Create 5 WAGED resources
+ String wagedResourcePrefix = "TEST_WAGED_DB_";
+ int wagedResourceCount = 5;
+ for (int i = 0; i < wagedResourceCount; i++) {
+ createWagedResource(wagedResourcePrefix + i,
+ DEFAULT_INSTANCE_CAPACITY * DEFAULT_INSTANCE_COUNT / REPLICAS / wagedResourceCount,
+ MIN_ACTIVE_REPLICAS, 100000L);
+ }
+
+ String crushedResourcePrefix = "TEST_CRUSHED_DB_";
+ int crushedResourceCount = 3;
+ for (int i = 0; i < crushedResourceCount; i++) {
+ createCrushedResource(crushedResourcePrefix + i,
+ DEFAULT_INSTANCE_CAPACITY * DEFAULT_INSTANCE_COUNT / REPLICAS / crushedResourceCount,
+ MIN_ACTIVE_REPLICA, 100000L);
+ }
+
+ // Wait for cluster to converge after adding resources
+ Assert.assertTrue(_clusterVerifier.verifyByPolling());
+
+ // Enter the cluster into MM
+ _gSetupTool.getClusterManagementTool()
+ .enableMaintenanceMode(CLUSTER_NAME, true,
+ "testComputePartitionAssignmentMaintenanceMode enters cluster into MM.");
+
+ // Add instance to cluster as enabled
+ String toAddInstanceName = "dummyInstance";
+ InstanceConfig toAddInstanceConfig = new InstanceConfig(toAddInstanceName);
+ toAddInstanceConfig.setInstanceCapacityMap(
+ Collections.singletonMap(INSTANCE_CAPACITY_KEY, DEFAULT_INSTANCE_CAPACITY));
+ _gSetupTool.getClusterManagementTool().addInstance(CLUSTER_NAME, toAddInstanceConfig);
+ _gSetupTool.getClusterManagementTool().enableInstance(CLUSTER_NAME, toAddInstanceName, true);
+
+ // Actually create the live instance
+ MockParticipantManager toAddParticipant = createParticipant(toAddInstanceName);
+ toAddParticipant.syncStart();
+
+ // Choose participant to simulate killing in API call
+ MockParticipantManager participantToKill = _participants.get(0);
+
+ // Use partition assignment API to simulate adding and killing separate instances
+ // returns idealStates for all resources in cluster
+ String payload = "{\"InstanceChange\" : { \"ActivateInstances\" : [\"" + toAddInstanceName
+ + "\"], \"DeactivateInstances\" : [\"" + participantToKill.getInstanceName() + "\"] }, "
+ + "\"Options\" : { \"ReturnFormat\" : \"IdealStateFormat\" }}";
+ Response response = post(getPartitionAssignmentPath(), null,
+ Entity.entity(payload, MediaType.APPLICATION_JSON_TYPE), Response.Status.OK.getStatusCode(),
+ true);
+ String body = response.readEntity(String.class);
+ Map<String, Map<String, Map<String, String>>> partitionAssignmentIdealStates =
+ OBJECT_MAPPER.readValue(body,
+ // Map of resources --> map of partitions --> MAP of instances --> state
+ new TypeReference<HashMap<String, Map<String, Map<String, String>>>>() {
+ });
+
+ // Kill the instance we simulated in partitionAssignment API and wait for delay window
+ participantToKill.syncStop();
+ Thread.sleep(3000L);
+
+ // Exit the cluster from MM
+ _gSetupTool.getClusterManagementTool()
+ .enableMaintenanceMode(CLUSTER_NAME, false,
+ "testComputePartitionAssignmentMaintenanceMode exits cluster out of MM.");
+
+ // Wait for cluster to converge after exiting maintenance mode
+ Assert.assertTrue(_clusterVerifier.verifyByPolling());
+
+ // Assert that all resource idealStates calculated by the partitionAssignment API during MM
+ // is identical to the idealStates calculated by the controller after it exits MM
Review Comment:
nit: too many spaces
##########
helix-rest/src/test/java/org/apache/helix/rest/server/TestPartitionAssignmentAPI.java:
##########
@@ -353,4 +355,114 @@ private void createWagedResource(String db, int numPartition, int minActiveRepli
Assert.assertTrue(_clusterVerifier.verifyByPolling());
}
+
+ private void createCrushedResource(String db, int numPartition, int minActiveReplica, long delay) {
+ _gSetupTool.addResourceToCluster(CLUSTER_NAME, db, numPartition, "LeaderStandby",
+ IdealState.RebalanceMode.FULL_AUTO + "", null);
+ _resources.add(db);
+
+ IdealState idealState =
+ _gSetupTool.getClusterManagementTool().getResourceIdealState(CLUSTER_NAME, db);
+ idealState.setMinActiveReplicas(minActiveReplica);
+ idealState.setDelayRebalanceEnabled(true);
+ idealState.setRebalanceDelay(delay);
+ idealState.setRebalancerClassName(DelayedAutoRebalancer.class.getName());
+ idealState.setRebalanceStrategy(CrushEdRebalanceStrategy.class.getName());
+ _gSetupTool.getClusterManagementTool().setResourceIdealState(CLUSTER_NAME, db, idealState);
+
+ ResourceConfig resourceConfig = new ResourceConfig(db);
+ _configAccessor.setResourceConfig(CLUSTER_NAME, db, resourceConfig);
+ _gSetupTool.rebalanceResource(CLUSTER_NAME, db, REPLICAS);
+
+ Assert.assertTrue(_clusterVerifier.verifyByPolling());
+ }
+ @Test
Review Comment:
nit: Missing newline.
--
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] Allow partitionAssignment API in maintenance mode [helix]
Posted by "GrantPSpencer (via GitHub)" <gi...@apache.org>.
GrantPSpencer commented on PR #2742:
URL: https://github.com/apache/helix/pull/2742#issuecomment-1909084103
> Overall LGTM 👍 Some minor formatting comments.
Good eye, fixed these in recent commit. Thanks!
--
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