-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test for topology migration by resource group #2933
Conversation
…gration which can isolate shuffling to a single resource group.
helix-core/src/test/java/org/apache/helix/integration/controller/TestTopologyMigration.java
Outdated
Show resolved
Hide resolved
_gSetupTool.getClusterManagementTool().enableMaintenanceMode(CLUSTER_NAME, false); | ||
|
||
// Verify cluster state after topology migration | ||
Assert.assertTrue(_clusterVerifier.verifyByPolling()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. If the main thread move fast and check the moment controller did not change the mapping, it may pass this line right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verifyByPolling
will wait to verify for .setWaitTillVerify(TestHelper.DEFAULT_REBALANCE_PROCESSING_WAIT_TIME)
which is 1500 milliseconds.
I looked into other ways to see if controller has processed that MM is set and found no other indicator given that MAINTENANCE znode and maintenance history in CONTROLLER/HISTORY are both written by the client.
Shouldn't verifyByPolling
wait long enough with the configured waitTillVerify
to allow the controller to process MM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verifybypolling only checks IS == EV. If there is no difference between IS and EV then it will pass.
It has the possibility controller move slow and no IS / EV diff generated at the moment MM exited. Then it can move to next line immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to using BestPossibleExternalViewVerifier that runs before cluster gets set to MM to ensure the cluster is converged with what verifier says is eventual BPS given the current state of the cluster and after we exit MM to ensure that cluster converges with the eventual BPS given the current state.
This will work because BestPossibleExternalViewVerifier actually computes BPS instead of just comparing IS and EV.
helix-core/src/test/java/org/apache/helix/integration/controller/TestTopologyMigration.java
Outdated
Show resolved
Hide resolved
…g with test logic.
…iew verifier to ensure that the cluster is converged with the bps that should eventually be calculated by the controller once it processes current state of the cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
This PR is ready to be merged. Final commit message:
|
Issues
Tests
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)