pkuwm commented on a change in pull request #1066:
URL: https://github.com/apache/helix/pull/1066#discussion_r456993778
##########
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:
@jiajunwang
> can we actually add the session Id to the ClusterEvent when it is
constructed, and then use that session Id directly here?
I don't think it is accurate: when the event is constructed and we add this
session id **S0** to event, if the manager session id changes to **S1** when
handling the event, the event would not be processed. But actually I think it
would be more accurate to use the session id when starting to process the
pipeline.
Or even session id changes to S1 and the event is not handled, there will be
future events to keep the correctness. In this case, I think it is fine to add
the session id to when event is created. But I think it may affect a bit
performance: originally event0 will process the pipeline, but it is discarded
and next event1 will be processed later.
So I think it is better to add the session right before running the pipeline.
##########
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:
I've thought about it. `data instanceof Message` adds one more check.
```
if (!data instanceof Message) {
return null;
}
ZNRecord record = (ZNRecord) data;
...
```
Since most of the time, this API is for message and we need to cast the
type, I prefer this so no need to always do the `instanceof` check.
##########
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:
Yep, I agree we should separate helix and zk logic cleanly. Let me
change and see if this will make it cleaner.
##########
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:
For this fix, we only need it for creating messages. For updating
messages, it should be done by participants. So we don't need that (correct me
if I am wrong). If we want to do it for all the write/create operations, we
have to protect all the async/sync operations, which I don't think we want to
do now.
If in the future we also need to protect write operations, we could
definitely quickly add the same logic as in create. 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]
