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/08/21 00:13:28 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1295: fix TestRawZkClient unstableness
jiajunwang commented on a change in pull request #1295:
URL: https://github.com/apache/helix/pull/1295#discussion_r474336659
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -124,6 +125,13 @@
private PathBasedZkSerializer _pathBasedZkSerializer;
private ZkClientMonitor _monitor;
+ final private String _monitorKey;
Review comment:
These fields are not helpful to the ZkClient.
Could you try to change the ZkClientMonitor instead so as to avoid these fields? What I am thinking is that, allowing the _eventThread sub-monitor to be added later after the ZkClientMonitor has been constructed. So it can be done in the connect() by referring to _monitor.
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
##########
@@ -285,18 +285,26 @@ public void testZkClientMonitor()
Assert.assertTrue(beanServer.isRegistered(idealStatename));
Assert.assertEquals((long) beanServer.getAttribute(name, "DataChangeEventCounter"), 0);
- Assert.assertEquals((long) beanServer.getAttribute(name, "StateChangeEventCounter"), 0);
+ Assert.assertEquals((long) beanServer.getAttribute(name, "StateChangeEventCounter"), 1);
Assert.assertEquals((long) beanServer.getAttribute(name, "ExpiredSessionCounter"), 0);
Assert.assertEquals((long) beanServer.getAttribute(name, "OutstandingRequestGauge"), 0);
// account for doAsyncSync()
Assert.assertEquals((long) beanServer.getAttribute(name, "TotalCallbackCounter"), 1);
- // Test exists
- Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadCounter"), 0);
- Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadTotalLatencyCounter"), 0);
- Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadLatencyGauge.Max"), 0);
- zkClient.exists(TEST_ROOT);
+ // Note, we need to wait here for the reason that doAsyncSync() blocks only the zkClient event thread. The main
+ // thread of zkClient would issue exits(TEST_ROOT) without blocking. The return of doAsyncSync() would be asyc
+ // to main thread. doAsyncSync() is a source of 1 read and main thread exists(TEST_ROOT) would be another.
+ TestHelper.verify(()->{
+ return ((org.apache.helix.zookeeper.zkclient.ZkClient)zkClient).getSyncStatus();
+ }, TestHelper.WAIT_DURATION);
+
Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadCounter"), 1);
+ //Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadTotalLatencyCounter"), 1);
Review comment:
Do we really need this check? If we ensure after exists(), the read is 2, then we shall be good, right?
Note that to check this read counter one by one, we need to add one method just for this purpose. I think it is an overkill and it is not necessary.
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/TestHelper.java
##########
@@ -32,7 +32,7 @@
public class TestHelper {
private static final Logger LOG = LoggerFactory.getLogger(TestHelper.class);
- public static final long WAIT_DURATION = 20 * 1000L; // 20 seconds
+ public static final long WAIT_DURATION = 60 * 1000L; // 20 seconds
Review comment:
1. the comment is not updated accordingly.
2. Could you please justify the reason we need to triple the wait duration?
##########
File path: zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
##########
@@ -285,18 +285,26 @@ public void testZkClientMonitor()
Assert.assertTrue(beanServer.isRegistered(idealStatename));
Assert.assertEquals((long) beanServer.getAttribute(name, "DataChangeEventCounter"), 0);
- Assert.assertEquals((long) beanServer.getAttribute(name, "StateChangeEventCounter"), 0);
+ Assert.assertEquals((long) beanServer.getAttribute(name, "StateChangeEventCounter"), 1);
Assert.assertEquals((long) beanServer.getAttribute(name, "ExpiredSessionCounter"), 0);
Assert.assertEquals((long) beanServer.getAttribute(name, "OutstandingRequestGauge"), 0);
// account for doAsyncSync()
Assert.assertEquals((long) beanServer.getAttribute(name, "TotalCallbackCounter"), 1);
- // Test exists
- Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadCounter"), 0);
- Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadTotalLatencyCounter"), 0);
- Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadLatencyGauge.Max"), 0);
- zkClient.exists(TEST_ROOT);
+ // Note, we need to wait here for the reason that doAsyncSync() blocks only the zkClient event thread. The main
+ // thread of zkClient would issue exits(TEST_ROOT) without blocking. The return of doAsyncSync() would be asyc
+ // to main thread. doAsyncSync() is a source of 1 read and main thread exists(TEST_ROOT) would be another.
+ TestHelper.verify(()->{
+ return ((org.apache.helix.zookeeper.zkclient.ZkClient)zkClient).getSyncStatus();
+ }, TestHelper.WAIT_DURATION);
+
Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadCounter"), 1);
+ //Assert.assertEquals((long) beanServer.getAttribute(rootname, "ReadTotalLatencyCounter"), 1);
Review comment:
What happen to these 2 checks?
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1281,6 +1279,10 @@ protected void doRetry() throws Exception {
});
}
+ public boolean getSyncStatus() {
Review comment:
I have multiple questions.
1. If this is just for test, please make it package-private.
2. Do we really need to check the read count one by one increase?
----------------------------------------------------------------
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