You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "himanshukandwal (via GitHub)" <gi...@apache.org> on 2024/01/11 21:30:30 UTC
[PR] [apache/helix] -- Improve CI/CD Performance [helix]
himanshukandwal opened a new pull request, #2735:
URL: https://github.com/apache/helix/pull/2735
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
In this PR, we are addressing the overall helix CI/CD performance
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
Changes to Boost performance include:
* Using a standardized Helix ZK Client (TestHelper#createZkClient) with shorter timeouts (ConnectInitTimeout, OperationRetryTimeout, SessionTimeout)
* Removed un-necessary Thread.sleep (especially after syncStop and syncStart statements).
* Avoided adding all the BuiltInStateModelDefinitions unnecessarily. Added on-demand bases, depending on the resource type.
Improved Runtime: 1h 30m
Previous Runtime: 2h 10m
### Tests
- [x] 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)
- 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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1470075163
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -278,16 +281,18 @@ public static void setupCluster(String clusterName, String zkAddr, int startPort
public static void setupCluster(String clusterName, String zkAddr, int startPort,
String participantNamePrefix, String resourceNamePrefix, int resourceNb, int partitionNb,
int nodesNb, int replica, String stateModelDef, RebalanceMode mode, boolean doRebalance) {
- HelixZkClient zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddr));
+ HelixZkClient zkClient = createZkClient(zkAddr);
try {
if (zkClient.exists("/" + clusterName)) {
LOG.warn("Cluster already exists:" + clusterName + ". Deleting it");
zkClient.deleteRecursively("/" + clusterName);
}
ClusterSetup setupTool = new ClusterSetup(zkAddr);
- setupTool.addCluster(clusterName, true);
+ List<BuiltInStateModelDefinitions> stateModelDefinitions = Objects.isNull(stateModelDef)
+ ? (resourceNb == 0 ? Collections.emptyList() : Arrays.asList(BuiltInStateModelDefinitions.values()))
+ : Arrays.asList(BuiltInStateModelDefinitions.valueOf(stateModelDef));
+ setupTool.addCluster(clusterName, stateModelDefinitions, false);
Review Comment:
Same here. I don't know why we would like to have tests on non existing state model?
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
Review Comment:
We do have the default value. Let's not hardcode it here.
##########
helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java:
##########
@@ -221,14 +221,19 @@ public void finalize() {
close();
}
- public void addCluster(String clusterName, boolean overwritePrevious, CloudConfig cloudConfig)
+ public void addCluster(String clusterName, boolean overwritePrevious, CloudConfig cloudConfig) {
+ addCluster(clusterName, Arrays.asList(BuiltInStateModelDefinitions.values()), overwritePrevious, cloudConfig);
+ }
+
+ public void addCluster(
+ String clusterName, List<BuiltInStateModelDefinitions> stateModelDefs, boolean overwritePrevious, CloudConfig cloudConfig)
Review Comment:
Why we need this?
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -76,8 +79,10 @@
public class TestHelper {
private static final Logger LOG = LoggerFactory.getLogger(TestHelper.class);
- public static final long WAIT_DURATION = 60 * 1000L; // 60 seconds
- public static final int DEFAULT_REBALANCE_PROCESSING_WAIT_TIME = 1500;
+ public static final long WAIT_DURATION = 10 * 1000L; // 10 seconds
+ public static final long POLL_DURATION = 10L; // 10 milli-seconds
+ public static final int DEFAULT_REBALANCE_PROCESSING_WAIT_TIME = 1000;
Review Comment:
Why we want to change this? Do we expect to fail it quicker?
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
+
+ HelixZkClient.ZkConnectionConfig zkConnectionConfig = new HelixZkClient.ZkConnectionConfig(zkAddress)
+ .setSessionTimeout(1000);
+
+ return DedicatedZkClientFactory.getInstance().buildZkClient(zkConnectionConfig, clientConfig);
+ }
+
+ public static void sleepQuietly(Duration duration) {
+ try {
+ Thread.sleep(duration.toMillis());
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ }
+
+ public static void timeIt(Runnable runnable) {
+ long startTime = System.currentTimeMillis();
+ try {
+ runnable.run();
+ } catch (Exception e) {
+ throw e;
+ } finally {
+ System.out.println(" -- time taken: " + (System.currentTimeMillis() - startTime) + "ms");
Review Comment:
Use log?
##########
helix-core/src/test/java/org/apache/helix/TestListenerCallbackBatchMode.java:
##########
@@ -221,21 +219,15 @@ public void testMixedListener() throws Exception {
}
private void verifyNonbatchedListeners(final Listener listener) throws Exception {
- Boolean result = TestHelper.verify(new TestHelper.Verifier() {
- @Override public boolean verify() {
- return (listener._instanceConfigChangedCount == _numNode) && (
- listener._idealStateChangedCount == _numResource);
- }
- }, 12000);
+ Boolean result = TestHelper.verify(() -> (listener._instanceConfigChangedCount == _numNode)
+ && (listener._idealStateChangedCount == _numResource), 4000);
Review Comment:
Why change timeout helps? If everything completes earlier, the timeout should not matter.
##########
helix-core/src/test/java/org/apache/helix/controller/stages/TestRebalancePipeline.java:
##########
@@ -157,7 +157,6 @@ public void testDuplicateMsg() throws Exception {
Assert.assertTrue(messages.get(0).getFromState().equalsIgnoreCase("SLAVE"));
Assert.assertTrue(messages.get(0).getToState().equalsIgnoreCase("MASTER"));
- Thread.sleep(2 * MessageGenerationPhase.DEFAULT_OBSELETE_MSG_PURGE_DELAY);
Review Comment:
Why we remove this? It was designed for delay message purge, right?
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
+
+ HelixZkClient.ZkConnectionConfig zkConnectionConfig = new HelixZkClient.ZkConnectionConfig(zkAddress)
+ .setSessionTimeout(1000);
+
+ return DedicatedZkClientFactory.getInstance().buildZkClient(zkConnectionConfig, clientConfig);
+ }
+
+ public static void sleepQuietly(Duration duration) {
+ try {
+ Thread.sleep(duration.toMillis());
+ } catch (InterruptedException e) {
Review Comment:
This usually means the test stops. Does print stack trace help?
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488875667
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
+
+ HelixZkClient.ZkConnectionConfig zkConnectionConfig = new HelixZkClient.ZkConnectionConfig(zkAddress)
+ .setSessionTimeout(1000);
+
+ return DedicatedZkClientFactory.getInstance().buildZkClient(zkConnectionConfig, clientConfig);
+ }
+
+ public static void sleepQuietly(Duration duration) {
+ try {
+ Thread.sleep(duration.toMillis());
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ }
+
+ public static void timeIt(Runnable runnable) {
+ long startTime = System.currentTimeMillis();
+ try {
+ runnable.run();
+ } catch (Exception e) {
+ throw e;
+ } finally {
+ System.out.println(" -- time taken: " + (System.currentTimeMillis() - startTime) + "ms");
Review Comment:
I have removed this function as this was something I used for measurement of individual pieces of execution.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488881680
##########
helix-core/src/test/java/org/apache/helix/integration/manager/ClusterManager.java:
##########
@@ -34,7 +34,7 @@
public class ClusterManager extends ZKHelixManager implements Runnable, ZkTestManager {
private static Logger LOG = LoggerFactory.getLogger(ClusterControllerManager.class);
- private static final int DISCONNECT_WAIT_TIME_MS = 3000;
+ private static final int DISCONNECT_WAIT_TIME_MS = 300;
Review Comment:
This value is used by all the participants (using MockParticipantManager) to finish off/disconnect when the cluster teardown is happening. Reducing this so that current thread is released faster.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488876782
##########
helix-core/src/test/java/org/apache/helix/TestListenerCallbackBatchMode.java:
##########
@@ -221,21 +219,15 @@ public void testMixedListener() throws Exception {
}
private void verifyNonbatchedListeners(final Listener listener) throws Exception {
- Boolean result = TestHelper.verify(new TestHelper.Verifier() {
- @Override public boolean verify() {
- return (listener._instanceConfigChangedCount == _numNode) && (
- listener._idealStateChangedCount == _numResource);
- }
- }, 12000);
+ Boolean result = TestHelper.verify(() -> (listener._instanceConfigChangedCount == _numNode)
+ && (listener._idealStateChangedCount == _numResource), 4000);
Review Comment:
Yes, I have reverted this change as timeout wont matter.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on PR #2735:
URL: https://github.com/apache/helix/pull/2735#issuecomment-1967835774
Ready for reviewing?
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488874460
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -76,8 +79,10 @@
public class TestHelper {
private static final Logger LOG = LoggerFactory.getLogger(TestHelper.class);
- public static final long WAIT_DURATION = 60 * 1000L; // 60 seconds
- public static final int DEFAULT_REBALANCE_PROCESSING_WAIT_TIME = 1500;
+ public static final long WAIT_DURATION = 10 * 1000L; // 10 seconds
+ public static final long POLL_DURATION = 10L; // 10 milli-seconds
+ public static final int DEFAULT_REBALANCE_PROCESSING_WAIT_TIME = 1000;
Review Comment:
I have reverted this change to keep the original values for now.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488882014
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
+
+ HelixZkClient.ZkConnectionConfig zkConnectionConfig = new HelixZkClient.ZkConnectionConfig(zkAddress)
+ .setSessionTimeout(1000);
+
+ return DedicatedZkClientFactory.getInstance().buildZkClient(zkConnectionConfig, clientConfig);
+ }
+
+ public static void sleepQuietly(Duration duration) {
+ try {
+ Thread.sleep(duration.toMillis());
+ } catch (InterruptedException e) {
Review Comment:
The benefit of using sleepQuietly was to avoid handling `InterruptedException` which is a checked exception. I have removed this for now.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488879623
##########
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java:
##########
@@ -738,7 +731,7 @@ protected void deleteCluster(String clusterName) {
*/
protected <T extends HelixProperty> T pollForProperty(Class<T> clazz, HelixDataAccessor accessor,
PropertyKey key, boolean shouldExist) throws InterruptedException {
- final int POLL_TIMEOUT = 5000;
+ final int POLL_TIMEOUT = 2000;
Review Comment:
Yeah, I have started with reducing it but this codepath is not used in many places so reducing this further has no effect.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488873812
##########
helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java:
##########
@@ -221,14 +221,19 @@ public void finalize() {
close();
}
- public void addCluster(String clusterName, boolean overwritePrevious, CloudConfig cloudConfig)
+ public void addCluster(String clusterName, boolean overwritePrevious, CloudConfig cloudConfig) {
+ addCluster(clusterName, Arrays.asList(BuiltInStateModelDefinitions.values()), overwritePrevious, cloudConfig);
+ }
+
+ public void addCluster(
+ String clusterName, List<BuiltInStateModelDefinitions> stateModelDefs, boolean overwritePrevious, CloudConfig cloudConfig)
Review Comment:
Yes, so we are creating a function here that creates the cluster loading by selectively loading the state-model defs.
This is needed for the an optimization that we are utilizing during every test cluster creation. We have found that loading all state-model defs everytime 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.
I have added as part of the java doc as well.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488876378
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
+
+ HelixZkClient.ZkConnectionConfig zkConnectionConfig = new HelixZkClient.ZkConnectionConfig(zkAddress)
+ .setSessionTimeout(1000);
+
+ return DedicatedZkClientFactory.getInstance().buildZkClient(zkConnectionConfig, clientConfig);
+ }
+
+ public static void sleepQuietly(Duration duration) {
+ try {
+ Thread.sleep(duration.toMillis());
+ } catch (InterruptedException e) {
Review Comment:
I have removed this function, this is not needed anymore.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488874460
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -76,8 +79,10 @@
public class TestHelper {
private static final Logger LOG = LoggerFactory.getLogger(TestHelper.class);
- public static final long WAIT_DURATION = 60 * 1000L; // 60 seconds
- public static final int DEFAULT_REBALANCE_PROCESSING_WAIT_TIME = 1500;
+ public static final long WAIT_DURATION = 10 * 1000L; // 10 seconds
+ public static final long POLL_DURATION = 10L; // 10 milli-seconds
+ public static final int DEFAULT_REBALANCE_PROCESSING_WAIT_TIME = 1000;
Review Comment:
I have reverted this change to keep the original values for now. Will take up in next iteration if we need to fine tune these.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488877926
##########
helix-core/src/test/java/org/apache/helix/controller/stages/TestRebalancePipeline.java:
##########
@@ -157,7 +157,6 @@ public void testDuplicateMsg() throws Exception {
Assert.assertTrue(messages.get(0).getFromState().equalsIgnoreCase("SLAVE"));
Assert.assertTrue(messages.get(0).getToState().equalsIgnoreCase("MASTER"));
- Thread.sleep(2 * MessageGenerationPhase.DEFAULT_OBSELETE_MSG_PURGE_DELAY);
Review Comment:
This was taking a constant time (2 min) from the pipeline run, which I think is too much. I will take up in next iteration and see if we can tune 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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488875382
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
Review Comment:
Yes, correct. I have removed now.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on PR #2735:
URL: https://github.com/apache/helix/pull/2735#issuecomment-1969971593
@himanshukandwal
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on PR #2735:
URL: https://github.com/apache/helix/pull/2735#issuecomment-1997897068
Hey @junkaixue, this PR is ready to be reviewed.
Latest full CI successful run: https://github.com/himanshukandwal/helix/actions/runs/8277053505
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1525490257
##########
helix-core/src/test/java/org/apache/helix/integration/TestErrorReplicaPersist.java:
##########
@@ -103,10 +103,9 @@ public void testErrorReplicaPersist() throws InterruptedException {
ClusterConfig clusterConfig = configAccessor.getClusterConfig(CLUSTER_NAME);
clusterConfig.setErrorPartitionThresholdForLoadBalance(100000);
configAccessor.setClusterConfig(CLUSTER_NAME, clusterConfig);
-
+
for (int i = 0; i < (NODE_NR + 1) / 2; i++) {
_participants[i].syncStop();
- Thread.sleep(2000);
Review 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.
##########
helix-core/src/test/java/org/apache/helix/integration/TestSchemataSM.java:
##########
@@ -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
Review Comment:
Use LeaderStandby instead of non-inclusive one.
##########
helix-core/src/test/java/org/apache/helix/integration/TestSchemataSM.java:
##########
@@ -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");
Review Comment:
Same here. If you touch it, better change the model
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488875270
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -278,16 +281,18 @@ public static void setupCluster(String clusterName, String zkAddr, int startPort
public static void setupCluster(String clusterName, String zkAddr, int startPort,
String participantNamePrefix, String resourceNamePrefix, int resourceNb, int partitionNb,
int nodesNb, int replica, String stateModelDef, RebalanceMode mode, boolean doRebalance) {
- HelixZkClient zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddr));
+ HelixZkClient zkClient = createZkClient(zkAddr);
try {
if (zkClient.exists("/" + clusterName)) {
LOG.warn("Cluster already exists:" + clusterName + ". Deleting it");
zkClient.deleteRecursively("/" + clusterName);
}
ClusterSetup setupTool = new ClusterSetup(zkAddr);
- setupTool.addCluster(clusterName, true);
+ List<BuiltInStateModelDefinitions> stateModelDefinitions = Objects.isNull(stateModelDef)
+ ? (resourceNb == 0 ? Collections.emptyList() : Arrays.asList(BuiltInStateModelDefinitions.values()))
+ : Arrays.asList(BuiltInStateModelDefinitions.valueOf(stateModelDef));
+ setupTool.addCluster(clusterName, stateModelDefinitions, false);
Review 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.
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal closed pull request #2735: [apache/helix] -- Improve CI/CD Performance
URL: https://github.com/apache/helix/pull/2735
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "MarkGaox (via GitHub)" <gi...@apache.org>.
MarkGaox commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1470558265
##########
helix-core/src/test/java/org/apache/helix/integration/manager/ClusterManager.java:
##########
@@ -34,7 +34,7 @@
public class ClusterManager extends ZKHelixManager implements Runnable, ZkTestManager {
private static Logger LOG = LoggerFactory.getLogger(ClusterControllerManager.class);
- private static final int DISCONNECT_WAIT_TIME_MS = 3000;
+ private static final int DISCONNECT_WAIT_TIME_MS = 300;
Review Comment:
Any particular reason for reducing this?
##########
helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java:
##########
@@ -738,7 +731,7 @@ protected void deleteCluster(String clusterName) {
*/
protected <T extends HelixProperty> T pollForProperty(Class<T> clazz, HelixDataAccessor accessor,
PropertyKey key, boolean shouldExist) throws InterruptedException {
- final int POLL_TIMEOUT = 5000;
+ final int POLL_TIMEOUT = 2000;
Review Comment:
Curious, why 2000 not 1000?
##########
helix-core/src/test/java/org/apache/helix/TestHelper.java:
##########
@@ -864,4 +866,36 @@ public static void printZkListeners(HelixZkClient client) throws Exception {
}
System.out.println("}");
}
+
+ public static HelixZkClient createZkClient(String zkAddress) {
+ HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig()
+ .setZkSerializer(new org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer())
+ .setConnectInitTimeout(1000L)
+ .setOperationRetryTimeout(1000L);
+
+ HelixZkClient.ZkConnectionConfig zkConnectionConfig = new HelixZkClient.ZkConnectionConfig(zkAddress)
+ .setSessionTimeout(1000);
+
+ return DedicatedZkClientFactory.getInstance().buildZkClient(zkConnectionConfig, clientConfig);
+ }
+
+ public static void sleepQuietly(Duration duration) {
+ try {
+ Thread.sleep(duration.toMillis());
+ } catch (InterruptedException e) {
Review Comment:
I'm curious about what's the benefit of using sleepQuietly vs Thread.sleep()?
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1470537665
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestMultiThreadStressTest/TestMultiThreadStressZKClient.java:
##########
@@ -52,7 +52,7 @@ private void setUp() {
public void testCreatePuppy() {
_zkMetaClient.create(zkParentKey, "test");
- PuppySpec puppySpec = new PuppySpec(PuppyMode.REPEAT, 0.2f, new ExecDelay(5000, 0.1f), 5);
+ PuppySpec puppySpec = new PuppySpec(PuppyMode.REPEAT, 0.2f, new ExecDelay(500, 0.1f), 5);
Review Comment:
Maybe change to a static final?
--
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
Re: [PR] [apache/helix] -- Improve CI/CD Performance [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2735:
URL: https://github.com/apache/helix/pull/2735#discussion_r1488878155
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestMultiThreadStressTest/TestMultiThreadStressZKClient.java:
##########
@@ -52,7 +52,7 @@ private void setUp() {
public void testCreatePuppy() {
_zkMetaClient.create(zkParentKey, "test");
- PuppySpec puppySpec = new PuppySpec(PuppyMode.REPEAT, 0.2f, new ExecDelay(5000, 0.1f), 5);
+ PuppySpec puppySpec = new PuppySpec(PuppyMode.REPEAT, 0.2f, new ExecDelay(500, 0.1f), 5);
Review Comment:
Yes, added this as a constant (static final)
--
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