Re: [PR] Create condition based rebalancer [helix]
junkaixue merged PR #2846: URL: https://github.com/apache/helix/pull/2846 -- 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-unsubscr...@helix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
Re: [PR] Create condition based rebalancer [helix]
frankmu commented on PR #2846: URL: https://github.com/apache/helix/pull/2846#issuecomment-2251362618 This PR is ready to be merged. Approved by @junkaixue Commit message: [Create condition based rebalancer] -- 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-unsubscr...@helix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
Re: [PR] Create condition based rebalancer [helix]
frankmu commented on code in PR #2846:
URL: https://github.com/apache/helix/pull/2846#discussion_r1690582066
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/condition/RebalanceConditionBuilder.java:
##
@@ -0,0 +1,49 @@
+package org.apache.helix.controller.rebalancer.condition;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BooleanSupplier;
+
+public class RebalanceConditionBuilder {
Review Comment:
Sure, can you share some code pointers on how to detect the config change or
topology change?
e.g. for config change, is it something like the following:
```
event.getEventType() == ClusterEventType.ClusterConfigChange
```
--
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-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
Re: [PR] Create condition based rebalancer [helix]
junkaixue commented on code in PR #2846:
URL: https://github.com/apache/helix/pull/2846#discussion_r1690432962
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/condition/RebalanceCondition.java:
##
@@ -0,0 +1,5 @@
+package org.apache.helix.controller.rebalancer.condition;
+
+public interface RebalanceCondition {
+ boolean shouldPerformRebalance();
Review Comment:
I think we need a generic string -> string map for input ? Or directly pass
the cache object.
Because condition is a piece of code logic but the real computation is
happening at runtime with input.
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/condition/RebalanceConditionBuilder.java:
##
@@ -0,0 +1,49 @@
+package org.apache.helix.controller.rebalancer.condition;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BooleanSupplier;
+
+public class RebalanceConditionBuilder {
Review Comment:
Would you like to have different default condition impls?
For example, config change based condition, topology change based condition.
--
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-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
Re: [PR] Create condition based rebalancer [helix]
frankmu commented on code in PR #2846:
URL: https://github.com/apache/helix/pull/2846#discussion_r1690257388
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/ConditionBasedRebalancer.java:
##
@@ -0,0 +1,179 @@
+package org.apache.helix.controller.rebalancer;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.rebalancer.condition.RebalanceCondition;
+import org.apache.helix.controller.stages.CurrentStateOutput;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.Resource;
+import org.apache.helix.model.ResourceAssignment;
+import org.apache.helix.model.StateModelDefinition;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ConditionBasedRebalancer extends
AbstractRebalancer {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConditionBasedRebalancer.class);
+ private final RebalanceCondition _rebalanceCondition;
+
+ public ConditionBasedRebalancer(RebalanceCondition rebalanceCondition) {
Review Comment:
updated
--
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-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
Re: [PR] Create condition based rebalancer [helix]
frankmu commented on code in PR #2846:
URL: https://github.com/apache/helix/pull/2846#discussion_r1689019136
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/ConditionBasedRebalancer.java:
##
@@ -0,0 +1,179 @@
+package org.apache.helix.controller.rebalancer;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.rebalancer.condition.RebalanceCondition;
+import org.apache.helix.controller.stages.CurrentStateOutput;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.Resource;
+import org.apache.helix.model.ResourceAssignment;
+import org.apache.helix.model.StateModelDefinition;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ConditionBasedRebalancer extends
AbstractRebalancer {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConditionBasedRebalancer.class);
+ private final RebalanceCondition _rebalanceCondition;
+
+ public ConditionBasedRebalancer(RebalanceCondition rebalanceCondition) {
Review Comment:
I guess this is already generic where we can specify simple or complex
conditions. If you check the PR description, we can do the following:
Sample code to trigger the rebalance with condition `if (event type is
CurrentStateChange OR CurrentStateChange)`
```
RebalanceCondition condition = new RebalanceConditionBuilder()
.addCondition(() ->
event.getEventType().equals(ClusterEventType.ClusterConfigChange))
.addCondition(() ->
event.getEventType().equals(ClusterEventType.CurrentStateChange))
.buildOr();
Rebalancer rebalancer = new ConditionBasedRebalancer(condition);
```
We can also do nested condition as following:
```
BooleanSupplier condition1 = () -> 5 > 3;
BooleanSupplier condition2 = () -> 2 > 3;
BooleanSupplier condition3 = () ->
event.getEventType().equals(ClusterEventType.ClusterConfigChange);
BooleanSupplier condition4 = () ->
event.getEventType().equals(ClusterEventType.CurrentStateChange);
RebalanceCondition andCondition = new RebalanceConditionBuilder()
.addCondition(condition1)
.addCondition(condition3)
.buildAnd();
RebalanceCondition orCondition = new RebalanceConditionBuilder()
.addCondition(condition2)
.addCondition(condition4)
.buildOr();
RebalanceCondition nestedCondition = new RebalanceConditionBuilder()
.addAndCondition(andCondition, orCondition)
.buildAnd();
Rebalancer rebalancer = new ConditionBasedRebalancer(nestedCondition);
```
Let me know how do you think, open to any suggestions
--
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-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
Re: [PR] Create condition based rebalancer [helix]
frankmu commented on code in PR #2846:
URL: https://github.com/apache/helix/pull/2846#discussion_r1689016395
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/condition/RebalanceCondition.java:
##
@@ -0,0 +1,5 @@
+package org.apache.helix.controller.rebalancer.condition;
+
+public interface RebalanceCondition {
+ boolean evaluate();
Review Comment:
Since the return type is a boolean, I have renamed it to
`shouldPerformRebalance`, but let me know if this doesn't work well.
--
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-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
Re: [PR] Create condition based rebalancer [helix]
junkaixue commented on code in PR #2846:
URL: https://github.com/apache/helix/pull/2846#discussion_r1688967238
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/ConditionBasedRebalancer.java:
##
@@ -0,0 +1,179 @@
+package org.apache.helix.controller.rebalancer;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.rebalancer.condition.RebalanceCondition;
+import org.apache.helix.controller.stages.CurrentStateOutput;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.Resource;
+import org.apache.helix.model.ResourceAssignment;
+import org.apache.helix.model.StateModelDefinition;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ConditionBasedRebalancer extends
AbstractRebalancer {
+ private static final Logger LOG =
LoggerFactory.getLogger(ConditionBasedRebalancer.class);
+ private final RebalanceCondition _rebalanceCondition;
+
+ public ConditionBasedRebalancer(RebalanceCondition rebalanceCondition) {
+this._rebalanceCondition = rebalanceCondition;
+ }
+
+ @Override
+ public IdealState computeNewIdealState(String resourceName, IdealState
currentIdealState,
+ CurrentStateOutput currentStateOutput, ResourceControllerDataProvider
clusterData) {
+if (!this._rebalanceCondition.evaluate()) {
+ ZNRecord cachedIdealState =
clusterData.getCachedOndemandIdealState(resourceName);
+ if (cachedIdealState != null) {
+return new IdealState(cachedIdealState);
+ }
+ // In theory, the cache should be populated already if no rebalance is
needed
+ LOG.warn(String.format(
Review Comment:
Better use log with '{}' for string construction instead of String.format.
The legacy code if we have time we need to refactor out as well.
##
helix-core/src/main/java/org/apache/helix/controller/rebalancer/ConditionBasedRebalancer.java:
##
@@ -0,0 +1,179 @@
+package org.apache.helix.controller.rebalancer;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.HelixException;
+import
org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.controller.rebalancer.condition.RebalanceCondition;
+import org.apache.helix.controller.stages.CurrentStateOutput;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.Resource;
+import org.apache.helix.model.ResourceAssignment;
+import org.apache.helix.model.StateModelDefinition;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ConditionBasedRebalancer extends
AbstractRebalancer {
+ private s
