You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2021/01/21 18:22:30 UTC
[GitHub] [helix] jiajunwang opened a new pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
jiajunwang opened a new pull request #1619:
URL: https://github.com/apache/helix/pull/1619
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
Resolves #1618
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This PR adds a cluster-level configuration for the supercluster that defines the default rebalance configuration. When Helix users activate the cluster using the HelixAdmin.activateCluster(), the API will read this config and apply the default rebalance configuration to the new cluster resource in the supercluster.
### Tests
- [X] The following tests are written for this issue:
TestClusterConfig.testSuperClusterRebalanceConfig
TestAddClusterV2.testRebalanceConfig and TestAddClusterV2.testInvalidRebalanceConfig
- 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.)
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r568263192
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
Which code convention are we talking about? I think there is no such rule... But please share a link so I can read it.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -134,7 +137,10 @@
// offline for more than this specified time period, and users call purge participant API,
// then the node will be removed.
// The unit is milliseconds.
- OFFLINE_DURATION_FOR_PURGE_MS
+ OFFLINE_DURATION_FOR_PURGE_MS,
+
+ // Resource rebalance configuration for the super cluster which manages Helix controller resources.
+ SUPER_CLUSTER_REBALANCE_CONFIG
Review comment:
My argument is that a sub-cluster can be a supercluster at the same time. It is possible to do a cascading setup. So for a sub-cluster, the additional setup still means a supercluster configuration, which will be used when it acts as a supercluster.
About the first suggestion, are you talking about use WAGED as the default rebalancer for the sub-cluster resource directly? That was the original design. I just have a concern about backward compatibility. But if you think it is fine, I'm totally fine with this simpler solution.
Regarding the other option, I thought about modifying the activeCluster API to accept the optional parameters. But this means we will need to modify helix-rest and helix-front as well. And there is a backward compatibility issue. I would rather deprecate this API and restrict it to only use the general resource creation APIs for activating a sub-cluster.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -134,7 +137,10 @@
// offline for more than this specified time period, and users call purge participant API,
// then the node will be removed.
// The unit is milliseconds.
- OFFLINE_DURATION_FOR_PURGE_MS
+ OFFLINE_DURATION_FOR_PURGE_MS,
+
+ // Resource rebalance configuration for the super cluster which manages Helix controller resources.
+ SUPER_CLUSTER_REBALANCE_CONFIG
Review comment:
As discussed with @dasahcc offline, we will just modify the default rebalance option for the sub-clusters. This will simplify this PR quite a lot. 👍
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -167,6 +173,14 @@
private static final int GLOBAL_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
private static final int OFFLINE_NODE_TIME_OUT_FOR_MAINTENANCE_MODE_NOT_SET = -1;
+ private static final Map<String, String> DEFAULT_SUPER_CLUSTER_REBALANCE_CONFIG = ImmutableMap
+ .of(RebalanceConfig.RebalanceConfigProperty.REBALANCE_MODE.name(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO.name(),
+ RebalanceConfig.RebalanceConfigProperty.REBALANCER_CLASS_NAME.name(),
+ DelayedAutoRebalancer.class.getName(),
Review comment:
@NealSun96 as discussed with Junkai, we decided to make WAGED the default option : )
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
This line is not necessary with the latest code change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564013199
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
Unnecessary else block
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -167,6 +173,14 @@
private static final int GLOBAL_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
private static final int OFFLINE_NODE_TIME_OUT_FOR_MAINTENANCE_MODE_NOT_SET = -1;
+ private static final Map<String, String> DEFAULT_SUPER_CLUSTER_REBALANCE_CONFIG = ImmutableMap
+ .of(RebalanceConfig.RebalanceConfigProperty.REBALANCE_MODE.name(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO.name(),
+ RebalanceConfig.RebalanceConfigProperty.REBALANCER_CLASS_NAME.name(),
+ DelayedAutoRebalancer.class.getName(),
Review comment:
I thought it's WAGED by default?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564050754
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1261,27 +1260,46 @@ public void addClusterToGrandCluster(String clusterName, String grandCluster) {
throw new HelixException("Cluster " + clusterName + " is not setup yet");
}
- IdealState idealState = new IdealState(clusterName);
+ ZKHelixDataAccessor accessor =
+ new ZKHelixDataAccessor(grandCluster, new ZkBaseDataAccessor<>(_zkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ ClusterConfig grandClusterConfig = accessor.getProperty(keyBuilder.clusterConfig());
+ RebalanceConfig superClusterRebalanceConfig =
+ grandClusterConfig.getSuperClusterRebalanceConfig();
+
+ // Note that the other rebalance configurations that are not verified below will be ignored.
Review comment:
I'm confused about this comment here. Does the following check validate all possible scenarios or not? Do you mean "other configuration" as valid or invalid?
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -393,4 +398,32 @@ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
// expected
}
}
+
+ @Test
+ public void testSuperClusterRebalanceConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ // Check the default values
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceStrategy(),
+ CrushEdRebalanceStrategy.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceClassName(),
+ DelayedAutoRebalancer.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceMode(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO);
+
+ // Check set and read
+ RebalanceConfig rebalanceConfig = testConfig.getSuperClusterRebalanceConfig();
+ rebalanceConfig.setRebalanceClassName(WagedRebalancer.class.getName());
Review comment:
Do we plan to also support REST API for this change? Feel that'll be very useful.
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -393,4 +398,32 @@ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
// expected
}
}
+
+ @Test
+ public void testSuperClusterRebalanceConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ // Check the default values
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceStrategy(),
+ CrushEdRebalanceStrategy.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceClassName(),
+ DelayedAutoRebalancer.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceMode(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO);
+
+ // Check set and read
+ RebalanceConfig rebalanceConfig = testConfig.getSuperClusterRebalanceConfig();
+ rebalanceConfig.setRebalanceClassName(WagedRebalancer.class.getName());
Review comment:
I think REST API through cluster config is good enough. Maybe you can add one test case in `TestClusterAccessor`, similar as `testPurgeOfflineParticipants` so that your PR is complete.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564013199
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
Unnecessary else block
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -167,6 +173,14 @@
private static final int GLOBAL_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
private static final int OFFLINE_NODE_TIME_OUT_FOR_MAINTENANCE_MODE_NOT_SET = -1;
+ private static final Map<String, String> DEFAULT_SUPER_CLUSTER_REBALANCE_CONFIG = ImmutableMap
+ .of(RebalanceConfig.RebalanceConfigProperty.REBALANCE_MODE.name(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO.name(),
+ RebalanceConfig.RebalanceConfigProperty.REBALANCER_CLASS_NAME.name(),
+ DelayedAutoRebalancer.class.getName(),
Review comment:
I thought it's WAGED by default?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r568273312
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -134,7 +137,10 @@
// offline for more than this specified time period, and users call purge participant API,
// then the node will be removed.
// The unit is milliseconds.
- OFFLINE_DURATION_FOR_PURGE_MS
+ OFFLINE_DURATION_FOR_PURGE_MS,
+
+ // Resource rebalance configuration for the super cluster which manages Helix controller resources.
+ SUPER_CLUSTER_REBALANCE_CONFIG
Review comment:
As discussed with @dasahcc offline, we will just modify the default rebalance option for the sub-clusters. This will simplify this PR quite a lot. 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang edited a comment on pull request #1619: Use WAGED rebalancer as the default rebalancer for the controller resources in the super cluster.
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1619:
URL: https://github.com/apache/helix/pull/1619#issuecomment-772062963
Tested locally and all tests passed. CI test is not stable now.
Approved by @dasahcc , I will merge the change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r568268714
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -134,7 +137,10 @@
// offline for more than this specified time period, and users call purge participant API,
// then the node will be removed.
// The unit is milliseconds.
- OFFLINE_DURATION_FOR_PURGE_MS
+ OFFLINE_DURATION_FOR_PURGE_MS,
+
+ // Resource rebalance configuration for the super cluster which manages Helix controller resources.
+ SUPER_CLUSTER_REBALANCE_CONFIG
Review comment:
My argument is that a sub-cluster can be a supercluster at the same time. It is possible to do a cascading setup. So for a sub-cluster, the additional setup still means a supercluster configuration, which will be used when it acts as a supercluster.
About the first suggestion, are you talking about use WAGED as the default rebalancer for the sub-cluster resource directly? That was the original design. I just have a concern about backward compatibility. But if you think it is fine, I'm totally fine with this simpler solution.
Regarding the other option, I thought about modifying the activeCluster API to accept the optional parameters. But this means we will need to modify helix-rest and helix-front as well. And there is a backward compatibility issue. I would rather deprecate this API and restrict it to only use the general resource creation APIs for activating a sub-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564089028
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -393,4 +398,32 @@ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
// expected
}
}
+
+ @Test
+ public void testSuperClusterRebalanceConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ // Check the default values
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceStrategy(),
+ CrushEdRebalanceStrategy.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceClassName(),
+ DelayedAutoRebalancer.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceMode(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO);
+
+ // Check set and read
+ RebalanceConfig rebalanceConfig = testConfig.getSuperClusterRebalanceConfig();
+ rebalanceConfig.setRebalanceClassName(WagedRebalancer.class.getName());
Review comment:
Rest API already supports modify cluster config. There is no need to add a specific method just for supercluster configuration, IMO. It is debatable for sure. But either way, we can do this PR first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564025160
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
It expresses the code logic better, IMO.
I can find many arguments online about this style, but I don't see much difference in this condition. If the previous return is for the exception or some early termination logic, then I think we should not use "else". But here, I don't see much difference. And my mind follows the current code better : )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r565592473
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -393,4 +398,32 @@ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
// expected
}
}
+
+ @Test
+ public void testSuperClusterRebalanceConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ // Check the default values
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceStrategy(),
+ CrushEdRebalanceStrategy.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceClassName(),
+ DelayedAutoRebalancer.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceMode(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO);
+
+ // Check set and read
+ RebalanceConfig rebalanceConfig = testConfig.getSuperClusterRebalanceConfig();
+ rebalanceConfig.setRebalanceClassName(WagedRebalancer.class.getName());
Review comment:
I doubt that it will increase any meaningful test coverage. 1. the REST API support is not the focus of this change. 2. There is no change in the REST, I think the existing tests cover the logic well.
To clarify, my point is that I don't have a good case that will pass the existing HelixAdmin tests but fail a REST test (because of a relevant change in this PR). So I think another REST test is just adding a boring layer to the already covered logic.
But if you do have such a case, please let me know and I will be glad to add the test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564018113
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -167,6 +173,14 @@
private static final int GLOBAL_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
private static final int OFFLINE_NODE_TIME_OUT_FOR_MAINTENANCE_MODE_NOT_SET = -1;
+ private static final Map<String, String> DEFAULT_SUPER_CLUSTER_REBALANCE_CONFIG = ImmutableMap
+ .of(RebalanceConfig.RebalanceConfigProperty.REBALANCE_MODE.name(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO.name(),
+ RebalanceConfig.RebalanceConfigProperty.REBALANCER_CLASS_NAME.name(),
+ DelayedAutoRebalancer.class.getName(),
Review comment:
We cannot change the existing default behavior. So this PR is to add the configuration item to make WAGED default.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
It expresses the code logic better, IMO.
I can find many arguments online about this style, but I don't see much difference in this condition. If the previous return is for the exception or some early termination logic, then I think we should not use "else". But here, I don't see much difference. And my mind follows the current code better : )
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1261,27 +1260,46 @@ public void addClusterToGrandCluster(String clusterName, String grandCluster) {
throw new HelixException("Cluster " + clusterName + " is not setup yet");
}
- IdealState idealState = new IdealState(clusterName);
+ ZKHelixDataAccessor accessor =
+ new ZKHelixDataAccessor(grandCluster, new ZkBaseDataAccessor<>(_zkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ ClusterConfig grandClusterConfig = accessor.getProperty(keyBuilder.clusterConfig());
+ RebalanceConfig superClusterRebalanceConfig =
+ grandClusterConfig.getSuperClusterRebalanceConfig();
+
+ // Note that the other rebalance configurations that are not verified below will be ignored.
Review comment:
It is not. Since we reused the existing structure RebalanceConfig, there are several fields that are not applicable.
That is also why I put the check in the API instead of the config class.
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -393,4 +398,32 @@ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
// expected
}
}
+
+ @Test
+ public void testSuperClusterRebalanceConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ // Check the default values
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceStrategy(),
+ CrushEdRebalanceStrategy.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceClassName(),
+ DelayedAutoRebalancer.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceMode(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO);
+
+ // Check set and read
+ RebalanceConfig rebalanceConfig = testConfig.getSuperClusterRebalanceConfig();
+ rebalanceConfig.setRebalanceClassName(WagedRebalancer.class.getName());
Review comment:
Rest API already supports modify cluster config. There is no need to add a specific method just for supercluster configuration, IMO. It is debatable for sure. But either way, we can do this PR first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564108336
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -393,4 +398,32 @@ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
// expected
}
}
+
+ @Test
+ public void testSuperClusterRebalanceConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ // Check the default values
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceStrategy(),
+ CrushEdRebalanceStrategy.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceClassName(),
+ DelayedAutoRebalancer.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceMode(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO);
+
+ // Check set and read
+ RebalanceConfig rebalanceConfig = testConfig.getSuperClusterRebalanceConfig();
+ rebalanceConfig.setRebalanceClassName(WagedRebalancer.class.getName());
Review comment:
I think REST API through cluster config is good enough. Maybe you can add one test case in `TestClusterAccessor`, similar as `testPurgeOfflineParticipants` so that your PR is complete.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r568169224
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -134,7 +137,10 @@
// offline for more than this specified time period, and users call purge participant API,
// then the node will be removed.
// The unit is milliseconds.
- OFFLINE_DURATION_FOR_PURGE_MS
+ OFFLINE_DURATION_FOR_PURGE_MS,
+
+ // Resource rebalance configuration for the super cluster which manages Helix controller resources.
+ SUPER_CLUSTER_REBALANCE_CONFIG
Review comment:
This could be confusing. Because other sub clusters can have this entry. What does it mean to the sub cluster? If this is only designed for super cluster. Would this be meaningful in ClusterConfig?
I have concerns to add it as specific for super cluster IS rebalance strategy. We should make it generic. Either we add it as default rebalance strategy for existing cluster. Or let's make the the configurable passing down from the admin API every time.
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
+1 to @NealSun96 . I would suggest to follow the coding convention.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang edited a comment on pull request #1619: Use WAGED rebalancer as the default rebalancer for the controller resources in the super cluster.
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1619:
URL: https://github.com/apache/helix/pull/1619#issuecomment-772062963
Tested locally and all tests passed. CI test is not stable now.
Approved by @dasahcc , I will merge the change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r568275156
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -167,6 +173,14 @@
private static final int GLOBAL_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
private static final int OFFLINE_NODE_TIME_OUT_FOR_MAINTENANCE_MODE_NOT_SET = -1;
+ private static final Map<String, String> DEFAULT_SUPER_CLUSTER_REBALANCE_CONFIG = ImmutableMap
+ .of(RebalanceConfig.RebalanceConfigProperty.REBALANCE_MODE.name(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO.name(),
+ RebalanceConfig.RebalanceConfigProperty.REBALANCER_CLASS_NAME.name(),
+ DelayedAutoRebalancer.class.getName(),
Review comment:
@NealSun96 as discussed with Junkai, we decided to make WAGED the default option : )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #1619: Use WAGED rebalancer as the default rebalancer for the controller resources in the super cluster.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1619:
URL: https://github.com/apache/helix/pull/1619
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1619: Use WAGED rebalancer as the default rebalancer for the controller resources in the super cluster.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1619:
URL: https://github.com/apache/helix/pull/1619#issuecomment-772062963
Approved by @dasahcc , I will merge the change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r568263192
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
Which code convention are we talking about? I think there is no such rule... But please share a link so I can read it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1619: Use WAGED rebalancer as the default rebalancer for the controller resources in the super cluster.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1619:
URL: https://github.com/apache/helix/pull/1619#issuecomment-772062963
Approved by @dasahcc , I will merge the change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564050754
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1261,27 +1260,46 @@ public void addClusterToGrandCluster(String clusterName, String grandCluster) {
throw new HelixException("Cluster " + clusterName + " is not setup yet");
}
- IdealState idealState = new IdealState(clusterName);
+ ZKHelixDataAccessor accessor =
+ new ZKHelixDataAccessor(grandCluster, new ZkBaseDataAccessor<>(_zkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ ClusterConfig grandClusterConfig = accessor.getProperty(keyBuilder.clusterConfig());
+ RebalanceConfig superClusterRebalanceConfig =
+ grandClusterConfig.getSuperClusterRebalanceConfig();
+
+ // Note that the other rebalance configurations that are not verified below will be ignored.
Review comment:
I'm confused about this comment here. Does the following check validate all possible scenarios or not? Do you mean "other configuration" as valid or invalid?
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterConfig.java
##########
@@ -393,4 +398,32 @@ private void trySetInvalidAbnormalStatesResolverMap(ClusterConfig testConfig,
// expected
}
}
+
+ @Test
+ public void testSuperClusterRebalanceConfig() {
+ ClusterConfig testConfig = new ClusterConfig("testConfig");
+
+ // Check the default values
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceStrategy(),
+ CrushEdRebalanceStrategy.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceClassName(),
+ DelayedAutoRebalancer.class.getName());
+ Assert.assertEquals(testConfig.getSuperClusterRebalanceConfig().getRebalanceMode(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO);
+
+ // Check set and read
+ RebalanceConfig rebalanceConfig = testConfig.getSuperClusterRebalanceConfig();
+ rebalanceConfig.setRebalanceClassName(WagedRebalancer.class.getName());
Review comment:
Do we plan to also support REST API for this change? Feel that'll be very useful.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564088431
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1261,27 +1260,46 @@ public void addClusterToGrandCluster(String clusterName, String grandCluster) {
throw new HelixException("Cluster " + clusterName + " is not setup yet");
}
- IdealState idealState = new IdealState(clusterName);
+ ZKHelixDataAccessor accessor =
+ new ZKHelixDataAccessor(grandCluster, new ZkBaseDataAccessor<>(_zkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ ClusterConfig grandClusterConfig = accessor.getProperty(keyBuilder.clusterConfig());
+ RebalanceConfig superClusterRebalanceConfig =
+ grandClusterConfig.getSuperClusterRebalanceConfig();
+
+ // Note that the other rebalance configurations that are not verified below will be ignored.
Review comment:
It is not. Since we reused the existing structure RebalanceConfig, there are several fields that are not applicable.
That is also why I put the check in the API instead of the config class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r564018113
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -167,6 +173,14 @@
private static final int GLOBAL_TARGET_TASK_THREAD_POOL_SIZE_NOT_SET = -1;
private static final int OFFLINE_NODE_TIME_OUT_FOR_MAINTENANCE_MODE_NOT_SET = -1;
+ private static final Map<String, String> DEFAULT_SUPER_CLUSTER_REBALANCE_CONFIG = ImmutableMap
+ .of(RebalanceConfig.RebalanceConfigProperty.REBALANCE_MODE.name(),
+ RebalanceConfig.RebalanceMode.FULL_AUTO.name(),
+ RebalanceConfig.RebalanceConfigProperty.REBALANCER_CLASS_NAME.name(),
+ DelayedAutoRebalancer.class.getName(),
Review comment:
We cannot change the existing default behavior. So this PR is to add the configuration item to make WAGED default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #1619: Use WAGED rebalancer as the default rebalancer for the controller resources in the super cluster.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1619:
URL: https://github.com/apache/helix/pull/1619
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1619: Allow cutomizing default rebalance configuration for the supercluster resources.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1619:
URL: https://github.com/apache/helix/pull/1619#discussion_r568282222
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1018,6 +1032,22 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ public void setSuperClusterRebalanceConfig(RebalanceConfig rebalanceConfig) {
+ _record.setMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name(),
+ rebalanceConfig.getConfigsMap());
+ }
+
+ public RebalanceConfig getSuperClusterRebalanceConfig() {
+ Map<String, String> rebalanceConfigMap =
+ _record.getMapField(ClusterConfigProperty.SUPER_CLUSTER_REBALANCE_CONFIG.name());
+ if (rebalanceConfigMap != null) {
+ return new RebalanceConfig(rebalanceConfigMap);
+ } else {
Review comment:
This line is not necessary with the latest code change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org