You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "MarkGaox (via GitHub)" <gi...@apache.org> on 2023/08/14 19:12:07 UTC

[GitHub] [helix] MarkGaox opened a new pull request, #2595: Implement the on-demand rebalance service

MarkGaox opened a new pull request, #2595:
URL: https://github.com/apache/helix/pull/2595

   ### Issues
   
   - [X] My PR addresses the following Helix issues and references them in the PR description:
   Service implementation of #2594
   
   ### Description
   
   - [X] Here are some details about my PR, including screenshots of any UI changes:
   This PR implements the on-demand rebalance service logic. It introduces a new cluster level config which help Helix understand when to force offline/disabled to be rebalanced without respecting the delay window.
   
   
   ### Tests
   
   - [X] The following tests are written for this issue:
   
   * TestDelayedAutoRebalance,TestDelayedWagedRebalance,TestDelayedAutoRebalanceWithDisabledInstance,TestDelayedAutoRebalanceWithRackaware,TestDelayedWagedRebalanceWithDisabledInstance,TestDelayedWagedRebalanceWithRackaware,TestClusterConfig
   
   - The following is the result of the "mvn test" command on the appropriate module:
   
   ```
   [INFO] Tests run: 91, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 549.684 s - in TestSuite
   [INFO] 
   [INFO] Results:
   [INFO] 
   [INFO] Tests run: 91, 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 943 classes
   [INFO] ------------------------------------------------------------------------
   [INFO] BUILD SUCCESS
   [INFO] ------------------------------------------------------------------------
   [INFO] Total time:  09:18 min
   [INFO] Finished at: 2023-08-14T11:37:05-07:00
   [INFO] ------------------------------------------------------------------------
   
   ```
   
   (If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
   
   ### Changes that Break Backward Compatibility (Optional)
   
   - My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
   
   (Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
   
   ### Documentation (Optional)
   
   - In case of new functionality, my PR adds documentation in the following wiki page:
   
   (Link the GitHub wiki you added)
   
   ### Commits
   
   - My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
     1. Subject is separated from body by a blank line
     1. Subject is limited to 50 characters (not including Jira issue reference)
     1. Subject does not end with a period
     1. Subject uses the imperative mood ("add", not "adding")
     1. Body wraps at 72 characters
     1. Body explains "what" and "why", not "how"
   
   ### Code Quality
   
   - My diff has been formatted using helix-style.xml 
   (helix-style-intellij.xml if IntelliJ IDE is used)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] MarkGaox commented on pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on PR #2595:
URL: https://github.com/apache/helix/pull/2595#issuecomment-1684215024

   This PR has been reviewed and approved by @junkaixue and @xyuanlu  and is ready to be merged
   
   Final Commit Message:
   Implement the on-demand rebalance service that performs rebalancing for offline or disabled instances without respecting the delay configuration.
   


-- 
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

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296260980


##########
helix-core/src/test/java/org/apache/helix/integration/rebalancer/DelayedAutoRebalancer/TestDelayedAutoRebalanceWithDisabledInstance.java:
##########
@@ -292,6 +292,85 @@ public void testDisableDelayRebalanceInInstance() throws Exception {
     super.testDisableDelayRebalanceInInstance();
   }
 
+  @Test(dependsOnMethods = {"testDisableDelayRebalanceInInstance"})
+  @Override

Review Comment:
   Or do you think it would be better to factor that out? Well..I personally feel like not factoring this out could be more intuitive. And In the future if you have more features you wanna to `disabled` nodes only or `offline` nodes only, then you can modify the test accordingly without touching the counterparts (And I believe it's gonna happen shortly when you implementing node swag). Just my two cents. And we can sync up offline today. ^_^



-- 
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

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1295108063


##########
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java:
##########
@@ -356,6 +356,14 @@ protected void setDelayTimeInCluster(HelixZkClient zkClient, String clusterName,
     configAccessor.setClusterConfig(clusterName, clusterConfig);
   }
 
+  protected void setLastOnDemandRebalanceTimeInCluster(HelixZkClient zkClient,

Review Comment:
   Discussed offline. Won't make any change in this PR. But will open a separate PR to factor out each method in `ZkTestBase.java` and make them `static`.



-- 
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

[GitHub] [helix] xyuanlu commented on pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #2595:
URL: https://github.com/apache/helix/pull/2595#issuecomment-1677991676

   Could you please post the PR to branch `ApplicationCLusterManager ` ?


-- 
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

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1294938531


##########
helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java:
##########
@@ -151,7 +151,10 @@ public enum ClusterConfigProperty {
     HELIX_ENABLED_DISABLE_TIMESTAMP,
     HELIX_DISABLED_REASON,
     // disabled type should be a enum of org.apache.helix.constants.InstanceConstants.InstanceDisabledType
-    HELIX_DISABLED_TYPE
+    HELIX_DISABLED_TYPE,
+
+    // The last time when the on-demand rebalance is triggered.
+    LAST_ON_DEMAND_REBALANCE_TIMESTAMP

Review Comment:
   When there are two calls C1 and C2 and C1 < C2, it's true that C1 will be overwritten by C2. It's fine for the rebalance triggered by C1 to be based on C2 timestamp because it won't make any difference. All offline/disabled nodes are still going to be rebalanced as long as T1 < C2 && C2 < T3. As for the rebalance call of C2, when it takes place the cluster is already rebalanced, therefore making no changes. Generally, we don't expect users to make concurrent cluster management API calls (but I understand it's not a good idea to make such assumption hahaha....). Hope this explains your concern. 
   
   For now, we only trigger the cluster level rebalance. We won't implement the fine-grained control of selecting which set of servers to be rebalanced unless the user explicitly requests that.



-- 
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

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296604407


##########
helix-core/src/test/java/org/apache/helix/integration/rebalancer/DelayedAutoRebalancer/TestDelayedAutoRebalanceWithDisabledInstance.java:
##########
@@ -292,6 +292,85 @@ public void testDisableDelayRebalanceInInstance() throws Exception {
     super.testDisableDelayRebalanceInInstance();
   }
 
+  @Test(dependsOnMethods = {"testDisableDelayRebalanceInInstance"})
+  @Override

Review Comment:
   As discussed offline, removed the test cases for rack aware related classes and factored out the code. 



-- 
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

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1294946128


##########
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java:
##########
@@ -356,6 +356,14 @@ protected void setDelayTimeInCluster(HelixZkClient zkClient, String clusterName,
     configAccessor.setClusterConfig(clusterName, clusterConfig);
   }
 
+  protected void setLastOnDemandRebalanceTimeInCluster(HelixZkClient zkClient,

Review Comment:
   It's seems like `ZkTestBase` is only be used by it's subclasses. And all `protected` methods from `ZkTestBase` can be directly called by it's subclasses. So static is not needed?



-- 
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

[GitHub] [helix] himanshukandwal commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1294004458


##########
helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java:
##########
@@ -151,7 +151,10 @@ public enum ClusterConfigProperty {
     HELIX_ENABLED_DISABLE_TIMESTAMP,
     HELIX_DISABLED_REASON,
     // disabled type should be a enum of org.apache.helix.constants.InstanceConstants.InstanceDisabledType
-    HELIX_DISABLED_TYPE
+    HELIX_DISABLED_TYPE,
+
+    // The last time when the on-demand rebalance is triggered.
+    LAST_ON_DEMAND_REBALANCE_TIMESTAMP

Review Comment:
   Just curious, what happens in case of concurrent API calls? This field will be overwritten by the last caller. 
   
   Also, do we need to retain the context specific to a partition or resource, or will we trigger cluster level rebalance (for-all)?



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1294104803


##########
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java:
##########
@@ -356,6 +356,14 @@ protected void setDelayTimeInCluster(HelixZkClient zkClient, String clusterName,
     configAccessor.setClusterConfig(clusterName, clusterConfig);
   }
 
+  protected void setLastOnDemandRebalanceTimeInCluster(HelixZkClient zkClient,

Review Comment:
   Could be static



-- 
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

[GitHub] [helix] xyuanlu merged pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2595:
URL: https://github.com/apache/helix/pull/2595


-- 
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

[GitHub] [helix] junkaixue commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1294069994


##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -417,6 +432,18 @@ private static int getMinActiveReplica(ResourceControllerDataProvider clusterDat
             currentIdealState), currentIdealState, numReplica);
   }
 
+  private static boolean isInstanceForcedToBeRebalancded(Long offlineOrDisabledTime, long delay,

Review Comment:
   NIT: add some description here. For example, what's the behavior of lastOndemand means here.



##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -417,6 +432,18 @@ private static int getMinActiveReplica(ResourceControllerDataProvider clusterDat
             currentIdealState), currentIdealState, numReplica);
   }
 
+  private static boolean isInstanceForcedToBeRebalancded(Long offlineOrDisabledTime, long delay,
+      long lastOnDemandRebalanceTimestamp) {
+    if (lastOnDemandRebalanceTimestamp == -1 || offlineOrDisabledTime == null
+        || offlineOrDisabledTime <= 0) {
+      return false;
+    }
+    long delayedInactiveTime = offlineOrDisabledTime + delay;
+    return offlineOrDisabledTime < lastOnDemandRebalanceTimestamp
+        && lastOnDemandRebalanceTimestamp < Math.min(delayedInactiveTime,

Review Comment:
   discussed offline. this piece may not need.



##########
helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java:
##########
@@ -1173,4 +1177,22 @@ public String getInstanceHelixDisabledTimeStamp(String instanceName) {
     }
     return getDisabledInstances().get(instanceName);
   }
+
+  /**
+   * @return: a unix time that represents the last time the on demand rebalance is triggered on
+   * current cluster. Return -1 if the configuration doesn't have such record yet.

Review Comment:
   Usually, param/return is under the description



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1297784446


##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -129,15 +129,25 @@ private static Set<String> getActiveNodes(Set<String> allNodes, Set<String> live
   }
 
   /**
-   * @return The time when an offline or disabled instance should be treated as inactive.
-   * Return -1 if it is inactive now.
+   * Return the time when an offline or disabled instance should be treated as inactive. Return -1
+   * if it is inactive now or forced to be rebalanced by an on-demand rebalance.
+   *
+   * @return A timestamp that represents the expected inactive time of a node.
    */
   private static long getInactiveTime(String instance, Set<String> liveInstances, Long offlineTime,
       long delay, InstanceConfig instanceConfig, ClusterConfig clusterConfig) {
     long inactiveTime = Long.MAX_VALUE;
+    long lastOnDemandRebalanceTime = clusterConfig.getLastOnDemandRebalanceTimestamp();
 
-    // check the time instance went offline.
+    // Check if the given instance is offline
     if (!liveInstances.contains(instance)) {
+      // Check if the offline instance is forced to be rebalanced by an on-demand rebalance.
+      // If so, return it as an inactive instance.
+      if (isInstanceForcedToBeRebalanced(offlineTime, delay, lastOnDemandRebalanceTime)) {
+        return -1L;
+      }
+
+      // Check the time instance went offline.
       if (offlineTime != null && offlineTime > 0 && offlineTime + delay < inactiveTime) {
         inactiveTime = offlineTime + delay;
       }

Review Comment:
   nit: line 144-153 and 167-177 can be generalized into on helper function.



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296207358


##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -417,6 +432,39 @@ private static int getMinActiveReplica(ResourceControllerDataProvider clusterDat
             currentIdealState), currentIdealState, numReplica);
   }
 
+  /**
+   * Given the offline/disabled time, delay, and the last on-demand rebalance time, this method checks
+   * if the node associated with the offline/disabled time is forced to be rebalanced by the on-demand
+   * rebalance. There are several cases:
+   *  1. If either the last on-demand rebalance time or the offline/disabled time is unavailable, then
+   *     the node is not forced to be rebalanced.
+   *  2. If the current time surpasses the delayed offline/disabled time, then the node is not forced
+   *     to be rebalanced.
+   *  3. If the last on-demand rebalance time is before the offline/disabled time, the node is not
+   *     forced to be rebalanced.

Review Comment:
   We listed all cases when we are not doing on demand rebalance. Do you think it would be clearer if we only list the case what we include on0demand rebalance?



##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -417,6 +432,39 @@ private static int getMinActiveReplica(ResourceControllerDataProvider clusterDat
             currentIdealState), currentIdealState, numReplica);
   }
 
+  /**
+   * Given the offline/disabled time, delay, and the last on-demand rebalance time, this method checks
+   * if the node associated with the offline/disabled time is forced to be rebalanced by the on-demand
+   * rebalance. There are several cases:
+   *  1. If either the last on-demand rebalance time or the offline/disabled time is unavailable, then
+   *     the node is not forced to be rebalanced.
+   *  2. If the current time surpasses the delayed offline/disabled time, then the node is not forced
+   *     to be rebalanced.
+   *  3. If the last on-demand rebalance time is before the offline/disabled time, the node is not
+   *     forced to be rebalanced.

Review Comment:
   We listed all cases when we are not doing on demand rebalance. Do you think it would be clearer if we only list the case what we include on-demand rebalance?



-- 
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

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296251942


##########
helix-core/src/test/java/org/apache/helix/integration/rebalancer/DelayedAutoRebalancer/TestDelayedAutoRebalanceWithDisabledInstance.java:
##########
@@ -292,6 +292,85 @@ public void testDisableDelayRebalanceInInstance() throws Exception {
     super.testDisableDelayRebalanceInInstance();
   }
 
+  @Test(dependsOnMethods = {"testDisableDelayRebalanceInInstance"})
+  @Override

Review Comment:
   `testOnDemandRebalance` in `TestDelayedAutoRebalance.java` tests only offline nodes. `testOnDemandRebalance` in `TestDelayedAutoRebalanceWithDisabledInstance.java` is for disabled instances.



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296407955


##########
helix-core/src/test/java/org/apache/helix/integration/rebalancer/DelayedAutoRebalancer/TestDelayedAutoRebalanceWithDisabledInstance.java:
##########
@@ -292,6 +292,85 @@ public void testDisableDelayRebalanceInInstance() throws Exception {
     super.testDisableDelayRebalanceInInstance();
   }
 
+  @Test(dependsOnMethods = {"testDisableDelayRebalanceInInstance"})
+  @Override

Review Comment:
   The change we made in this PR is in how we filter out instances for further stages in pipeline.  Since no changes are made in further pipeline, we don't need to add test for each rebalance flavor.
   We do need to test when there are disabled or offline instances. We could test them in the same unit test. Having duplicate code is not a good practice. 



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1294104455


##########
helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java:
##########
@@ -1173,4 +1177,22 @@ public String getInstanceHelixDisabledTimeStamp(String instanceName) {
     }
     return getDisabledInstances().get(instanceName);
   }
+
+  /**
+   * @return: a unix time that represents the last time the on demand rebalance is triggered on
+   * current cluster. Return -1 if the configuration doesn't have such record yet.
+   */
+  public long getLastOnDemandRebalanceTimestamp() {
+    return _record.getLongField(ClusterConfigProperty.LAST_ON_DEMAND_REBALANCE_TIMESTAMP.name(),
+        LAST_ON_DEMAND_REBALANCE_TIMESTAMP_NOT_SET);

Review Comment:
   nit: may consider change name. Current name sounds like an exception name.



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296407955


##########
helix-core/src/test/java/org/apache/helix/integration/rebalancer/DelayedAutoRebalancer/TestDelayedAutoRebalanceWithDisabledInstance.java:
##########
@@ -292,6 +292,85 @@ public void testDisableDelayRebalanceInInstance() throws Exception {
     super.testDisableDelayRebalanceInInstance();
   }
 
+  @Test(dependsOnMethods = {"testDisableDelayRebalanceInInstance"})
+  @Override

Review Comment:
   The change we made in this PR is in how we filter out instances for further stages in pipeline.  Since no changes are made in further pipeline, we don't need to add test for every single rebalance flavor.
   We do need to test when there are disabled or offline instances. We could test them in the same unit test. Having duplicate code is not a good practice. 



-- 
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

[GitHub] [helix] GrantPSpencer commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "GrantPSpencer (via GitHub)" <gi...@apache.org>.
GrantPSpencer commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1293992640


##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -135,9 +135,17 @@ private static Set<String> getActiveNodes(Set<String> allNodes, Set<String> live
   private static long getInactiveTime(String instance, Set<String> liveInstances, Long offlineTime,
       long delay, InstanceConfig instanceConfig, ClusterConfig clusterConfig) {
     long inactiveTime = Long.MAX_VALUE;
+    long lastOnDemandRebalanceTime = clusterConfig.getLastOnDemandRebalanceTimestamp();
 
-    // check the time instance went offline.
+    // Check if the given instance is offline
     if (!liveInstances.contains(instance)) {
+      // Check if the offline instance is forced to be rebalanced by an on-demand rebalance.
+      // If so, return it as an inactive instance.
+      if (isInstanceForcedToBeRebalancded(offlineTime, delay, lastOnDemandRebalanceTime)) {

Review Comment:
   nit: typo 
   rebalancDed --> rebalanced



-- 
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

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1298685175


##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -129,15 +129,25 @@ private static Set<String> getActiveNodes(Set<String> allNodes, Set<String> live
   }
 
   /**
-   * @return The time when an offline or disabled instance should be treated as inactive.
-   * Return -1 if it is inactive now.
+   * Return the time when an offline or disabled instance should be treated as inactive. Return -1
+   * if it is inactive now or forced to be rebalanced by an on-demand rebalance.
+   *
+   * @return A timestamp that represents the expected inactive time of a node.
    */
   private static long getInactiveTime(String instance, Set<String> liveInstances, Long offlineTime,
       long delay, InstanceConfig instanceConfig, ClusterConfig clusterConfig) {
     long inactiveTime = Long.MAX_VALUE;
+    long lastOnDemandRebalanceTime = clusterConfig.getLastOnDemandRebalanceTimestamp();
 
-    // check the time instance went offline.
+    // Check if the given instance is offline
     if (!liveInstances.contains(instance)) {
+      // Check if the offline instance is forced to be rebalanced by an on-demand rebalance.
+      // If so, return it as an inactive instance.
+      if (isInstanceForcedToBeRebalanced(offlineTime, delay, lastOnDemandRebalanceTime)) {
+        return -1L;
+      }
+
+      // Check the time instance went offline.
       if (offlineTime != null && offlineTime > 0 && offlineTime + delay < inactiveTime) {
         inactiveTime = offlineTime + delay;
       }

Review Comment:
   Discussed offline. Will open a new PR to address this and the `static` keyword issue. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org

[GitHub] [helix] MarkGaox commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1295108063


##########
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java:
##########
@@ -356,6 +356,14 @@ protected void setDelayTimeInCluster(HelixZkClient zkClient, String clusterName,
     configAccessor.setClusterConfig(clusterName, clusterConfig);
   }
 
+  protected void setLastOnDemandRebalanceTimeInCluster(HelixZkClient zkClient,

Review Comment:
   Discussed offline. Won't make any 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.

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

[GitHub] [helix] zpinto commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296116918


##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -135,9 +135,17 @@ private static Set<String> getActiveNodes(Set<String> allNodes, Set<String> live
   private static long getInactiveTime(String instance, Set<String> liveInstances, Long offlineTime,

Review Comment:
   Can you update the description in the docstring of this function here to reflect how force rebalance affects inactive time?



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296209221


##########
helix-core/src/test/java/org/apache/helix/integration/rebalancer/DelayedAutoRebalancer/TestDelayedAutoRebalanceWithDisabledInstance.java:
##########
@@ -292,6 +292,85 @@ public void testDisableDelayRebalanceInInstance() throws Exception {
     super.testDisableDelayRebalanceInInstance();
   }
 
+  @Test(dependsOnMethods = {"testDisableDelayRebalanceInInstance"})
+  @Override

Review Comment:
   Following tests are bit duplicated. We already test on demand rebalance and test filtering out hosts in previous tests. 



-- 
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

[GitHub] [helix] junkaixue commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296452406


##########
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java:
##########
@@ -417,6 +432,39 @@ private static int getMinActiveReplica(ResourceControllerDataProvider clusterDat
             currentIdealState), currentIdealState, numReplica);
   }
 
+  /**
+   * Given the offline/disabled time, delay, and the last on-demand rebalance time, this method checks
+   * if the node associated with the offline/disabled time is forced to be rebalanced by the on-demand
+   * rebalance. There are several cases:
+   *  1. If either the last on-demand rebalance time or the offline/disabled time is unavailable, then
+   *     the node is not forced to be rebalanced.
+   *  2. If the current time surpasses the delayed offline/disabled time, then the node is not forced
+   *     to be rebalanced.
+   *  3. If the last on-demand rebalance time is before the offline/disabled time, the node is not
+   *     forced to be rebalanced.
+   *  4. If the last on-demand rebalance time is after the delayed offline/disabled time, then the
+   *     node is not forced to be rebalanced.
+   *  5. Otherwise, the offline/disabled node should be forced to rebalance.
+   *
+   * @param offlineOrDisabledTime A unix timestamp indicating the most recent time when a node went
+   *                              offline or was disabled.
+   * @param delay The delay window configuration of the current cluster
+   * @param lastOnDemandRebalanceTime A unix timestamp representing the most recent time when an
+   *                                  on-demand rebalance was triggered.
+   * @return a boolean indicating whether a node is forced to be rebalanced
+   */
+  private static boolean isInstanceForcedToBeRebalanced(Long offlineOrDisabledTime, long delay,
+      long lastOnDemandRebalanceTime) {
+    if (lastOnDemandRebalanceTime == -1 || offlineOrDisabledTime == null
+        || offlineOrDisabledTime <= 0 || System.currentTimeMillis() > (offlineOrDisabledTime
+        + delay)) {
+      return false;
+    }
+
+    return offlineOrDisabledTime < lastOnDemandRebalanceTime && lastOnDemandRebalanceTime < (

Review Comment:
   still:
   
   lastOnDemandRebalanceTime < (offlineOrDisabledTime + delay)
   
   As we discussed, even lastOnDemandRebalanceTime > (offlineOrDisabledTime + delay). It is OK. It means the time already passed. We don't need check this.



-- 
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

[GitHub] [helix] xyuanlu commented on a diff in pull request #2595: Implement the on-demand rebalance service

Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2595:
URL: https://github.com/apache/helix/pull/2595#discussion_r1296407955


##########
helix-core/src/test/java/org/apache/helix/integration/rebalancer/DelayedAutoRebalancer/TestDelayedAutoRebalanceWithDisabledInstance.java:
##########
@@ -292,6 +292,85 @@ public void testDisableDelayRebalanceInInstance() throws Exception {
     super.testDisableDelayRebalanceInInstance();
   }
 
+  @Test(dependsOnMethods = {"testDisableDelayRebalanceInInstance"})
+  @Override

Review Comment:
   The change we made in this PR is how we filter out instances for further stages in pipeline.  Since no changes are made in further pipeline, we don't need to add test for each rebalance flavor.
   We do need to test when there are disabled or offline instances. We could test them in the same unit test. Having duplicate code is not a good practice. 



-- 
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