pkuwm commented on a change in pull request #1066:
URL: https://github.com/apache/helix/pull/1066#discussion_r457089646
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -2234,13 +2205,59 @@ private String getHexSessionId() {
}
/*
- * Session aware operation needs below requirements:
- * 1. the session id is NOT null or empty
- * 2. create mode is EPHEMERAL or EPHEMERAL_SEQUENTIAL
+ * Gets the zookeeper instance that ensures its session ID matches the
expected session ID.
+ * It is used for write operations that suppose the znode to be created by
the expected session.
*/
- private boolean isSessionAwareOperation(String expectedSessionId, CreateMode
mode) {
- return expectedSessionId != null && !expectedSessionId.isEmpty() && (
- mode == CreateMode.EPHEMERAL || mode ==
CreateMode.EPHEMERAL_SEQUENTIAL);
+ private ZooKeeper getExpectedZookeeper(final String expectedSessionId) {
+ /*
+ * Cache the zookeeper reference and make sure later zooKeeper.create() is
being run
+ * under this zookeeper connection. This is to avoid zk session change
after expected
+ * session check.
+ */
+ ZooKeeper zk = ((ZkConnection) getConnection()).getZookeeper();
+
+ /*
+ * The operation is NOT session aware, we will use the actual zookeeper
session without
+ * checking expected session.
+ */
+ if (expectedSessionId == null || expectedSessionId.isEmpty()) {
+ return zk;
+ }
+
+ /*
+ * If operation is session aware (expectedSession is valid),
+ * we have to check whether or not the passed-in(expected) session id
+ * matches actual session's id.
+ * If not, we should not return a zk object for the zk operation.
+ */
+ final String actualSessionId = Long.toHexString(zk.getSessionId());
+ if (!actualSessionId.equals(expectedSessionId)) {
+ throw new ZkSessionMismatchedException(
+ "Failed to get expected zookeeper instance! There is a session id
mismatch. Expected: "
+ + expectedSessionId + ". Actual: " + actualSessionId);
+ }
+
+ return zk;
+ }
+
+ private String parseExpectedSessionId(Object data) {
+ if (data == null) {
+ return null;
+ }
+
+ ZNRecord record;
+ try {
+ record = (ZNRecord) data;
+ } catch (ClassCastException e) {
+ LOG.debug("Failed to parse expected session id!", e);
+ return null;
+ }
+
+ // Check it is a message and get src session id as expected session id for
message.
Review comment:
@jiajunwang I've cleaned up the code in zkClient. I think it is cleaner.
My thought is we don't have to create a new class extends ZNRecord which only
introduces an expectedSessionId. It is fine to just add the field/methods to
existing ZNRecord. And we move the expected session population logic to
ZkHelixDataAccessor's createChildren. It seems to me a clean way. What do you
think?
----------------------------------------------------------------
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]
