kaisun2000 commented on a change in pull request #1295:
URL: https://github.com/apache/helix/pull/1295#discussion_r474351453
##########
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:
Seems you have concern about this one by increase it one by one
https://github.com/apache/helix/pull/1295/files#r474338328
Let me share my thinking here.
The doAsyncSync() callback finishing is async to main thread here. It can
increase the read counter anytime. Thus, we need TestHelper.verify to wait for
this read from sync(). Here, delete line 301 assert is also ok, just wait for
307 having read counter as 2. Same reason, assert 301 is 1 is fine.
In sum, either way it is fine. But without TestHelper.verify waiting is not
fine.
##########
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:
1/ Will make this package level by removing public.
2/ See my comment
https://github.com/apache/helix/pull/1295/files#r474351453. See if this address
the concern.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
