Get rid of non-official ZK error code to avoid NPEs. (#1778)
This PR removes the non-official ZK error codes that are returned by the Helix ZkClient. The original design was made for differentiating the failure cases. However, that design caused unexpected NPE when most of Helix's logic relies on the Zookeeper KeeperException.Code class to identify the error types.
diff --git a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
index 8b742da..cbbdbbd 100644
--- a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
+++ b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
@@ -1276,7 +1276,7 @@
});
} catch (RuntimeException e) {
// Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.UNKNOWN_RET_CODE, path,
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
new ZkAsyncCallMonitorContext(_monitor, startT, 0, true));
throw e;
}
@@ -1960,14 +1960,9 @@
});
return null;
});
- } catch (ZkSessionMismatchedException e) {
- // Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.ZK_SESSION_MISMATCHED_CODE, path,
- new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
- throw e;
} catch (RuntimeException e) {
// Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.UNKNOWN_RET_CODE, path,
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
throw e;
}
@@ -2003,14 +1998,9 @@
});
return null;
});
- } catch (ZkSessionMismatchedException e) {
- // Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.ZK_SESSION_MISMATCHED_CODE, path,
- new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
- throw e;
} catch (RuntimeException e) {
// Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.UNKNOWN_RET_CODE, path,
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
throw e;
}
@@ -2031,7 +2021,7 @@
});
} catch (RuntimeException e) {
// Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.UNKNOWN_RET_CODE, path,
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
new ZkAsyncCallMonitorContext(_monitor, startT, 0, true), null, null);
throw e;
}
@@ -2052,7 +2042,7 @@
});
} catch (RuntimeException e) {
// Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.UNKNOWN_RET_CODE, path,
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
new ZkAsyncCallMonitorContext(_monitor, startT, 0, true), null);
throw e;
}
@@ -2073,7 +2063,7 @@
});
} catch (RuntimeException e) {
// Process callback to release caller from waiting
- cb.processResult(ZkAsyncCallbacks.UNKNOWN_RET_CODE, path,
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
new ZkAsyncCallMonitorContext(_monitor, startT, 0, false));
throw e;
}
diff --git a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
index c188e7e..7f662cf 100644
--- a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
+++ b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
@@ -35,13 +35,6 @@
public class ZkAsyncCallbacks {
private static Logger LOG = LoggerFactory.getLogger(ZkAsyncCallbacks.class);
- public static final int UNKNOWN_RET_CODE = 255;
-
- // Whenever there's a mismatch between the expected Zookeeper session ID and the actual
- // Zookeeper session ID for the async operation, the ZkClient which performs the async
- // operation passes this return code to the async callback indicate that it has caught
- // a ZkSessionMismatchedException.
- public static final int ZK_SESSION_MISMATCHED_CODE = 127;
public static class GetDataCallbackHandler extends DefaultCallback implements DataCallback {
public byte[] _data;
@@ -182,7 +175,7 @@
*/
public static abstract class DefaultCallback implements CancellableZkAsyncCallback {
AtomicBoolean _isOperationDone = new AtomicBoolean(false);
- int _rc = UNKNOWN_RET_CODE;
+ int _rc = KeeperException.Code.APIERROR.intValue();
public void callback(int rc, String path, Object ctx) {
if (rc != 0 && LOG.isDebugEnabled()) {
@@ -282,10 +275,6 @@
* @return true if the error is transient and the operation may succeed when being retried.
*/
protected boolean needRetry(int rc) {
- if (rc == ZK_SESSION_MISMATCHED_CODE) {
- LOG.error("Actual session ID doesn't match with expected session ID. Skip retrying.");
- return false;
- }
try {
switch (Code.get(rc)) {
/** Connection to the server has been lost */
diff --git a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
index e078367..79efd12 100644
--- a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
+++ b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestRawZkClient.java
@@ -887,7 +887,7 @@
// Ensure the async callback is cancelled because of the exception
Assert.assertTrue(createCallback.waitForSuccess(), "Callback operation should be done");
- Assert.assertEquals(createCallback.getRc(), ZkAsyncCallbacks.ZK_SESSION_MISMATCHED_CODE);
+ Assert.assertEquals(createCallback.getRc(), KeeperException.Code.APIERROR.intValue());
}
Assert.assertFalse(zkClient.exists(path));
@@ -915,7 +915,7 @@
// Ensure the async callback is cancelled because of the exception
Assert.assertTrue(setDataCallback.waitForSuccess(), "Callback operation should be done");
- Assert.assertEquals(setDataCallback.getRc(), ZkAsyncCallbacks.ZK_SESSION_MISMATCHED_CODE);
+ Assert.assertEquals(setDataCallback.getRc(), KeeperException.Code.APIERROR.intValue());
}
TestHelper.verify(() -> zkClient.delete(path), TestHelper.WAIT_DURATION);
diff --git a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestZkClientAsyncRetry.java b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestZkClientAsyncRetry.java
index 6845aa9..e55f8e6 100644
--- a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestZkClientAsyncRetry.java
+++ b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestZkClientAsyncRetry.java
@@ -42,7 +42,6 @@
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-import static org.apache.helix.zookeeper.zkclient.callback.ZkAsyncCallbacks.UNKNOWN_RET_CODE;
import static org.apache.zookeeper.KeeperException.Code.CONNECTIONLOSS;
/**
@@ -143,7 +142,7 @@
}
ZkAsyncCallbacks.CreateCallbackHandler createCallback =
new ZkAsyncCallbacks.CreateCallbackHandler();
- Assert.assertEquals(createCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(createCallback.getRc(), KeeperException.Code.APIERROR.intValue());
testZkClient.setAsyncCallRC(code.intValue());
if (code == CONNECTIONLOSS || code == KeeperException.Code.SESSIONEXPIRED
|| code == KeeperException.Code.SESSIONMOVED) {
@@ -190,7 +189,7 @@
// 1. Test async set retry
ZkAsyncCallbacks.SetDataCallbackHandler setCallback =
new ZkAsyncCallbacks.SetDataCallbackHandler();
- Assert.assertEquals(setCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(setCallback.getRc(), KeeperException.Code.APIERROR.intValue());
tmpRecord.setSimpleField("test", "data");
testZkClient.setAsyncCallRC(CONNECTIONLOSS.intValue());
@@ -214,7 +213,7 @@
// 2. Test async delete
ZkAsyncCallbacks.DeleteCallbackHandler deleteCallback =
new ZkAsyncCallbacks.DeleteCallbackHandler();
- Assert.assertEquals(deleteCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(deleteCallback.getRc(), KeeperException.Code.APIERROR.intValue());
testZkClient.setAsyncCallRC(CONNECTIONLOSS.intValue());
// Async delete will be pending due to the mock error rc is retryable.
@@ -254,7 +253,7 @@
// 1. Test async create retry
ZkAsyncCallbacks.CreateCallbackHandler createCallback =
new ZkAsyncCallbacks.CreateCallbackHandler();
- Assert.assertEquals(createCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(createCallback.getRc(), KeeperException.Code.APIERROR.intValue());
tmpRecord.setSimpleField("test", "data");
testZkClient.setAsyncCallRC(CONNECTIONLOSS.intValue());
@@ -280,7 +279,7 @@
// 1. Test async set retry
ZkAsyncCallbacks.SetDataCallbackHandler setCallback =
new ZkAsyncCallbacks.SetDataCallbackHandler();
- Assert.assertEquals(setCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(setCallback.getRc(), KeeperException.Code.APIERROR.intValue());
tmpRecord.setSimpleField("test", "data");
testZkClient.setAsyncCallRC(CONNECTIONLOSS.intValue());
@@ -317,7 +316,7 @@
// 1. Test async exist check
ZkAsyncCallbacks.ExistsCallbackHandler existsCallback =
new ZkAsyncCallbacks.ExistsCallbackHandler();
- Assert.assertEquals(existsCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(existsCallback.getRc(), KeeperException.Code.APIERROR.intValue());
testZkClient.setAsyncCallRC(CONNECTIONLOSS.intValue());
// Async exist check will be pending due to the mock error rc is retryable.
@@ -339,7 +338,7 @@
// 2. Test async get
ZkAsyncCallbacks.GetDataCallbackHandler getCallback =
new ZkAsyncCallbacks.GetDataCallbackHandler();
- Assert.assertEquals(getCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(getCallback.getRc(), KeeperException.Code.APIERROR.intValue());
testZkClient.setAsyncCallRC(CONNECTIONLOSS.intValue());
// Async get will be pending due to the mock error rc is retryable.
@@ -423,7 +422,7 @@
// Test asyncGet failure
ZkAsyncCallbacks.GetDataCallbackHandler getCallback =
new ZkAsyncCallbacks.GetDataCallbackHandler();
- Assert.assertEquals(getCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(getCallback.getRc(), KeeperException.Code.APIERROR.intValue());
// asyncGet should fail because the return code is APIERROR
testZkClient.setAsyncCallRC(KeeperException.Code.APIERROR.intValue());
testZkClient.asyncGetData(NODE_PATH, getCallback);
@@ -445,7 +444,7 @@
// Test asyncExists failure
ZkAsyncCallbacks.ExistsCallbackHandler existsCallback =
new ZkAsyncCallbacks.ExistsCallbackHandler();
- Assert.assertEquals(existsCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(existsCallback.getRc(), KeeperException.Code.APIERROR.intValue());
// asyncExists should fail because the return code is APIERROR
testZkClient.setAsyncCallRC(KeeperException.Code.APIERROR.intValue());
testZkClient.asyncExists(NODE_PATH, existsCallback);
@@ -467,7 +466,7 @@
// Test asyncSet failure
ZkAsyncCallbacks.SetDataCallbackHandler setCallback =
new ZkAsyncCallbacks.SetDataCallbackHandler();
- Assert.assertEquals(setCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(setCallback.getRc(), KeeperException.Code.APIERROR.intValue());
// asyncSet should fail because the return code is APIERROR
testZkClient.setAsyncCallRC(KeeperException.Code.APIERROR.intValue());
testZkClient.asyncSetData(NODE_PATH, tmpRecord, -1, setCallback);
@@ -481,7 +480,7 @@
// Test asyncDelete failure
ZkAsyncCallbacks.DeleteCallbackHandler deleteCallback =
new ZkAsyncCallbacks.DeleteCallbackHandler();
- Assert.assertEquals(deleteCallback.getRc(), UNKNOWN_RET_CODE);
+ Assert.assertEquals(deleteCallback.getRc(), KeeperException.Code.APIERROR.intValue());
// asyncDelete should fail because the return code is APIERROR
testZkClient.setAsyncCallRC(KeeperException.Code.APIERROR.intValue());
testZkClient.asyncDelete(NODE_PATH, deleteCallback);
