jiajunwang commented on a change in pull request #1295:
URL: https://github.com/apache/helix/pull/1295#discussion_r490637634
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -31,9 +31,11 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
import javax.management.JMException;
+import com.google.common.annotations.VisibleForTesting;
Review comment:
With the latest proposal, this one can be removed too.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -31,9 +31,11 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
Review comment:
no need anymore?
##########
File path:
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
##########
@@ -285,18 +285,20 @@ 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);
- Assert.assertEquals((long) beanServer.getAttribute(rootname,
"ReadCounter"), 1);
+
+ // Assert.assertEquals((long) beanServer.getAttribute(rootname,
"ReadCounter"), 2);
+ // wait for both doAysncSync() finish and zkClient.exists(Test_ROOT)
finish from 2 different threads.
+ // The condition would be ReadCounter is 2.
+ boolean verifyResult = TestHelper.verify(()->{
+ return (long) beanServer.getAttribute(rootname, "ReadCounter") == 2;
+ }, TestHelper.WAIT_DURATION);
+ Assert.assertTrue(verifyResult, " did not see 2 read yet");
+
Assert.assertTrue((long) beanServer.getAttribute(rootname,
"ReadTotalLatencyCounter") >= 0);
Assert.assertTrue((long) beanServer.getAttribute(rootname,
"ReadLatencyGauge.Max") >= 0);
Review comment:
If we checked "ReadLatencyGauge.Max" before, then this one can be
ignored.
##########
File path:
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
##########
@@ -285,18 +285,20 @@ 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);
Review comment:
Actually, can we wait here before "zkClient.exists(TEST_ROOT);" for
ReadCounter == 1. Then wait here for 2? So we ensure exist call will increase
one counter too.
##########
File path:
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
##########
@@ -285,18 +285,20 @@ 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);
Review comment:
If you wait for the counter in the previous line, then here we can still
check for
Assert.assertTrue((long) beanServer.getAttribute(rootname,
"ReadTotalLatencyCounter") >= 0);
Assert.assertTrue((long) beanServer.getAttribute(rootname,
"ReadLatencyGauge.Max") >= 0);
right?
##########
File path:
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
##########
@@ -285,18 +285,20 @@ 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);
- Assert.assertEquals((long) beanServer.getAttribute(rootname,
"ReadCounter"), 1);
+
+ // Assert.assertEquals((long) beanServer.getAttribute(rootname,
"ReadCounter"), 2);
+ // wait for both doAysncSync() finish and zkClient.exists(Test_ROOT)
finish from 2 different threads.
+ // The condition would be ReadCounter is 2.
+ boolean verifyResult = TestHelper.verify(()->{
+ return (long) beanServer.getAttribute(rootname, "ReadCounter") == 2;
+ }, TestHelper.WAIT_DURATION);
+ Assert.assertTrue(verifyResult, " did not see 2 read yet");
+
Assert.assertTrue((long) beanServer.getAttribute(rootname,
"ReadTotalLatencyCounter") >= 0);
Review comment:
I propose that we record the ReadTotalLatencyCounter that we get before
exists() call. Then we check if this counter is equal to the previous value or
is increased here.
----------------------------------------------------------------
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]
