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 2020/06/23 09:17:07 UTC
[GitHub] [helix] pkuwm commented on a change in pull request #1117: Fix test testMsgTriggeredRebalance
pkuwm commented on a change in pull request #1117:
URL: https://github.com/apache/helix/pull/1117#discussion_r444080381
##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/TestRebalancePipeline.java
##########
@@ -203,25 +189,25 @@ public void testMsgTriggeredRebalance() throws Exception {
Assert.assertTrue(TestHelper.verify(() -> {
allMsgs.clear();
for (LiveInstance liveInstance : liveInstances) {
- allMsgs.addAll(accessor.getChildValues(keyBuilder.messages(liveInstance.getInstanceName()),
- true));
- }
- if (allMsgs.size() != 1 || !allMsgs.get(0).getToState().equals("MASTER") || !allMsgs.get(0)
- .getFromState().equals("SLAVE")) {
- return false;
+ allMsgs.addAll(
+ accessor.getChildValues(keyBuilder.messages(liveInstance.getInstanceName()), true));
}
- return true;
+ // Because current state equals pending message's toState, pending message is ignored
+ // and ST messages are sent to localhost_0, messages number >= 1
+ return !allMsgs.isEmpty() && allMsgs.get(0).getToState().equals("MASTER") && allMsgs.get(0)
Review comment:
This is the change to pass the test, allowing messages size >= 1
##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/TestRebalancePipeline.java
##########
@@ -176,22 +163,22 @@ public void testMsgTriggeredRebalance() throws Exception {
// Since controller's rebalancer pipeline will GC pending messages after timeout, and both hosts
// update current states to SLAVE, controller will send out rebalance message to
// have one host to become master
- setCurrentState(clusterName, "localhost_0", resourceName, resourceName + "_0", liveInstances.get(0).getEphemeralOwner(),
- "SLAVE", true);
- setCurrentState(clusterName, "localhost_1", resourceName, resourceName + "_0", liveInstances.get(1).getEphemeralOwner(),
- "SLAVE", true);
+ setCurrentState(clusterName, "localhost_0", resourceName, resourceName + "_0",
+ liveInstances.get(0).getEphemeralOwner(), "SLAVE", true);
+ setCurrentState(clusterName, "localhost_1", resourceName, resourceName + "_0",
+ liveInstances.get(1).getEphemeralOwner(), "SLAVE", true);
// Controller has timeout > 1sec, so within 1s, controller should not have GCed message
Assert.assertTrue(msgPurgeDelay > 1000);
Assert.assertFalse(TestHelper.verify(() -> {
+ boolean isMessagePurged = false;
for (LiveInstance liveInstance : liveInstances) {
List<String> messages =
accessor.getChildNames(keyBuilder.messages(liveInstance.getInstanceName()));
- if (messages.size() >= 1) {
- return false;
- }
+ // Should verify messages on each live instance are not purged.
+ isMessagePurged = isMessagePurged || messages.isEmpty();
Review comment:
Change 1: verify messages on **each** live instance are not purged.
----------------------------------------------------------------
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