jiajunwang commented on a change in pull request #1066:
URL: https://github.com/apache/helix/pull/1066#discussion_r456593184
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1827,18 +1796,20 @@ public void asyncCreate(final String path, Object
datat, final CreateMode mode,
new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
return;
}
- doAsyncCreate(path, data, mode, startT, cb);
+ doAsyncCreate(path, data, mode, startT, cb, parseExpectedSessionId(datat));
Review comment:
So we only protect create but not write?
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -695,12 +695,19 @@ private void handleEvent(ClusterEvent event,
BaseControllerDataProvider dataProv
event.addAttribute(AttributeName.STATEFUL_REBALANCER.name(),
_rebalancerRef.getRebalancer(manager));
- if (!manager.isLeader()) {
- logger.error("Cluster manager: " + manager.getInstanceName() + " is not
leader for " + manager
- .getClusterName() + ". Pipeline will not be invoked");
+ Optional<String> leaderSession = manager.getSessionIdIfLead();
Review comment:
Sorry that if I asked this question before (this PR has been opened for
a long time), but can we actually add the session Id to the ClusterEvent when
it is constructed, and then use that session Id directly here?
Or they are actually different Session Ids. 1. is the id that from which the
HelixManger get events. 2. is the current HelixManager session Id.
If this is the case, then we shall not call it EVENT_SESSION. It would be
misleading.
I think that using the real EVENT session Id would be good enough. So we may
don't need this Manger Session Id here.
##########
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;
Review comment:
Will "data instanceof Message" be simpler here?
##########
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:
This logic is Helix logic specific, not a good idea to put in ZkClient.
To make it generic, one way is adding a new interface of ZnRecord
(SessionAwareZnRecordUpdateRequest, etc.) which provides a method such as
getExpectedSessionId() to return this information.
----------------------------------------------------------------
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]
