kaisun2000 commented on a change in pull request #1360:
URL: https://github.com/apache/helix/pull/1360#discussion_r488329131
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/TestAlertingRebalancerFailure.java
##########
@@ -118,18 +120,25 @@ public void beforeMethod() throws IOException {
@Test
public void testParticipantUnavailable() throws Exception {
- _gSetupTool.addResourceToCluster(CLUSTER_NAME, testDb, 5,
- BuiltInStateModelDefinitions.MasterSlave.name(),
RebalanceMode.FULL_AUTO.name());
+ IdealState idealState = new FullAutoModeISBuilder(testDb)
Review comment:
Let us make sure I understand this test fix correctly. With #1359 fix,
the net effect is that previous missing "rebalance error" metric is added.
Also, avoid setting empty best possible state to the `output` which is used by
later rebalance stage.
In this test, we did not assert anything about rebalance error metrics;
thus, even missing one rebalance failure metrics, it would cause this test not
stable. So why do we need the change in this test?
Put it another way, what caused the this test unstable?
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/TestAlertingRebalancerFailure.java
##########
@@ -161,7 +170,8 @@ public void testParticipantUnavailable() throws Exception {
@Test(dependsOnMethods = "testParticipantUnavailable")
public void testTagSetIncorrect() throws Exception {
_gSetupTool.addResourceToCluster(CLUSTER_NAME, testDb, 5,
- BuiltInStateModelDefinitions.MasterSlave.name(),
RebalanceMode.FULL_AUTO.name());
Review comment:
Previously, we use AutoRebalancer with AutoRebalanceStrategy. With this
change, we use DelayedAutoRebalancer with CrushEd strategy.
The question is that now, by default with this change, we will use
DelayedAutoRebalancer by default. If we forget to add CrushEdRebalanceStrategy,
the default seems to be AutoRebalanceStrategy. Will that work?
##########
File path:
helix-core/src/test/java/org/apache/helix/monitoring/mbeans/TestRebalancerMetrics.java
##########
@@ -128,8 +128,10 @@ public void testLoadBalanceMetrics() {
event.addAttribute(AttributeName.CURRENT_STATE.name(), currentStateOutput);
setupLiveInstances(4);
- runStage(event, new ReadClusterDataStage());
Review comment:
why remove this line?
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/TestAlertingRebalancerFailure.java
##########
@@ -118,18 +120,25 @@ public void beforeMethod() throws IOException {
@Test
public void testParticipantUnavailable() throws Exception {
- _gSetupTool.addResourceToCluster(CLUSTER_NAME, testDb, 5,
- BuiltInStateModelDefinitions.MasterSlave.name(),
RebalanceMode.FULL_AUTO.name());
+ IdealState idealState = new FullAutoModeISBuilder(testDb)
Review comment:
I see, this assertion may not work, if pipeline run another cycle?
```
pollForError(accessor, errorNodeKey);
checkRebalanceFailureGauge(true);
checkResourceBestPossibleCalFailureState(
ResourceMonitor.RebalanceStatus.BEST_POSSIBLE_STATE_CAL_FAILED,
testDb);
```
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
