pkuwm commented on a change in pull request #1119:
URL: https://github.com/apache/helix/pull/1119#discussion_r459166737
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -120,6 +122,39 @@ public void handle() {
}
}
+ public static class SyncCallbackHandler extends DefaultCallback implements
AsyncCallback.VoidCallback {
+ private String _sessionId;
+
+ public SyncCallbackHandler(String sessionId) {
+ _sessionId = sessionId;
+ }
+
+ @Override
+ public void processResult(int rc, String path, Object ctx) {
+ LOG.info("sycnOnNewSession with sessionID {} async return code: {}",
_sessionId, rc);
+ callback(rc, path, ctx);
+ }
+
+ @Override
+ public void handle() {
+ // Make compiler happy, not used.
+ }
+
+ @Override
+ protected boolean needRetry(int rc) {
+ try {
+ // Connection to the server has been lost
+ if (KeeperException.Code.get(rc) == Code.CONNECTIONLOSS) {
+ return true;
+ }
+ return false;
+ } catch (ClassCastException | NullPointerException ex) {
Review comment:
@kaisun2000 Could you help understand this: `KeeperException.Code.get()
can throw null point exception.` Underneath the lookup is a map. map.get(rc)
returns null if rc doesn't exist.
```
/**
* Get the Code value for a particular integer error code
* @param code int error code
* @return Code value corresponding to specified int code, or null
*/
public static Code get(int code) {
return lookup.get(code);
}
```
The code should be like
```
if (KeeperException.Code.get(rc) == Code.CONNECTIONLOSS) {
return true;
}
LOG.error("Session {} failed to handle unknown return code {}. Skip
retrying.",
_sessionId, rc);
return false;
```
##########
File path:
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
##########
@@ -553,6 +554,52 @@ public void testCreateEphemeralWithValidSession()
_zkClient.delete(path);
}
+ /*
+ * This test validates that when ZK_AUTOSYNC_ENABLED_DEFAULT is enabled,
sync() would be issued
+ * before handleNewSession. ZKclient would not see stale data.
+ */
+ @Test
+ public void testAutoSyncWithNewSessionEstablishment() throws Exception {
+ final String path = "/" + TestHelper.getTestMethodName();
+ final String data = "Hello Helix 2";
+
+ // Wait until the ZkClient has got a new session.
+ Assert.assertTrue(TestHelper
+ .verify(() ->
_zkClient.getConnection().getZookeeperState().isConnected(), 1000L));
+
+ final long originalSessionId = _zkClient.getSessionId();
+
+ try {
+ // Create node.
+ _zkClient.create(path, data, CreateMode.PERSISTENT);
+ } catch (Exception ex) {
+ Assert.fail("Failed to create ephemeral node.", ex);
+ }
+
+ // Expire the original session.
+ ZkTestHelper.expireSession(_zkClient);
+
+ // Wait until the ZkClient has got a new session.
+ Assert.assertTrue(TestHelper.verify(() -> {
+ try {
+ // New session id should not equal to expired session id.
+ return _zkClient.getSessionId() != originalSessionId;
+ } catch (ZkClientException ex) {
+ return false;
+ }
+ }, 1000L));
+
+ // Verify the node is created and its data is correct.
+ Stat stat = new Stat();
+ String nodeData = null;
+ try {
+ nodeData = _zkClient.readData(path, stat, true);
+ } catch (ZkException e) {
+ Assert.fail("fail to read data");
Review comment:
I meant, it is not about `it doesn't hurt`.
`ZkTestHelper.expireSession(_zkClient)` could also possibly throw exception.
Are you going to do this try...catch...fail?
If this test throws exception, it definitely will fail from the point where
exception is thrown.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -187,6 +196,10 @@ protected ZkClient(IZkConnection zkConnection, int
connectionTimeout, long opera
if (zkConnection == null) {
throw new NullPointerException("Zookeeper connection is null!");
}
+
+ String syncUpOnNewSession =
System.getProperty(ZkSystemPropertyKeys.ZK_AUTOSYNC_ENABLED,
ZK_AUTOSYNC_ENABLED_DEFAULT);
+ _syncOnNewSession = Boolean.parseBoolean(syncUpOnNewSession);
Review comment:
Nit. This could be a constant: `static final boolean SYNC_ON_NEW_SESSION
=
Boolean.parseBoolean(System.getProperty(ZkSystemPropertyKeys.ZK_AUTOSYNC_ENABLED,
ZK_AUTOSYNC_ENABLED_DEFAULT))`
----------------------------------------------------------------
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]
