[apache/helix] -- Improve CI/CD Performance #2735
Conversation
| List<BuiltInStateModelDefinitions> stateModelDefinitions = Objects.isNull(stateModelDef) | ||
| ? (resourceNb == 0 ? Collections.emptyList() : Arrays.asList(BuiltInStateModelDefinitions.values())) | ||
| : Arrays.asList(BuiltInStateModelDefinitions.valueOf(stateModelDef)); | ||
| setupTool.addCluster(clusterName, stateModelDefinitions, false); |
There was a problem hiding this comment.
Same here. I don't know why we would like to have tests on non existing state model?
There was a problem hiding this comment.
Yes, updated the reasoning with a comment as well along here. This is needed to avoid loading all the state-model defs and loading only the one which user has explicitly specified. Otherwise, we will load all.
helix-core/src/test/java/org/apache/helix/TestListenerCallbackBatchMode.java
Outdated
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/controller/stages/TestRebalancePipeline.java
Outdated
Show resolved
Hide resolved
...apache/helix/metaclient/impl/zk/TestMultiThreadStressTest/TestMultiThreadStressZKClient.java
Outdated
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/integration/manager/ClusterManager.java
Outdated
Show resolved
Hide resolved
|
Ready for reviewing? |
|
Hey @junkaixue, this PR is ready to be reviewed. Latest full CI successful run: https://github.com/himanshukandwal/helix/actions/runs/8277053505 |
| for (int i = 0; i < (NODE_NR + 1) / 2; i++) { | ||
| _participants[i].syncStop(); | ||
| Thread.sleep(2000); |
There was a problem hiding this comment.
Better not remove this, this wait time is for cluster converge. If you need to fix this, would be good to put a verifier here.
| @@ -56,7 +56,7 @@ public void testSchemataSM() throws Exception { | |||
| 1, // partitions per resource | |||
| n, // number of nodes | |||
| 0, // replicas | |||
| "STORAGE_DEFAULT_SM_SCHEMATA", false); // don't rebalance | |||
| "MasterSlave", false); // don't rebalance | |||
There was a problem hiding this comment.
Use LeaderStandby instead of non-inclusive one.
| @@ -101,7 +101,7 @@ public void testSchemataSM() throws Exception { | |||
| for (int i = 0; i < n; i++) { | |||
| String instanceName = "localhost_" + (12918 + i); | |||
| Assert.assertNotNull(stateMap.get(instanceName)); | |||
| Assert.assertEquals(stateMap.get(instanceName), "MASTER"); | |||
| Assert.assertEquals(stateMap.get(instanceName), i == 0 ? "MASTER" : "SLAVE"); | |||
There was a problem hiding this comment.
Same here. If you touch it, better change the model
Issues
In this PR, we are addressing the overall helix CI/CD performance
Description
Changes to Boost performance include:
1. Using a standardized Helix ZK Client (TestHelper#createZkClient)
Allowed only one way to create ZK client and avoiding code duplication.
2. Removed un-necessary Thread.sleep
We removed constant time Thread.sleep statements especially after methods that add implicit wait and ensure sync completion. These include:
3. Avoided adding all the BuiltInStateModelDefinitions
Allowed adding state-model defs on-demand bases during the cluster creation, depending on the resource type. This is an optimization that we are performing to selectively load only the state-model defs that are required for cluster. We have found that loading all state-model defs everytime in a cluster creation takes about 500-700ms, due to invocation of ZKUtil::isClusterSetup function that (repeatedly) ensure all the helix cluster paths are created before loading a state-model.
Improved Runtime: 1h 30m
Previous Runtime: 2h 10m
Tests
The following tests are written for this issue:
N/A
The following is the result of the "mvn test" command on the appropriate module:
(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)
(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)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)