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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
