jiajunwang commented on a change in pull request #1066:
URL: https://github.com/apache/helix/pull/1066#discussion_r441920425
##########
File path: helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java
##########
@@ -95,6 +95,24 @@
*/
boolean[] createChildren(List<String> paths, List<T> records, int options);
+ /**
+ * Use it when creating children under a parent node with an expected ZK
session.
+ * <p>
+ * This will use async api for better performance. If the children already
exist it will return
+ * false.
+ *
+ * @param paths the paths to the children ZNodes
+ * @param records List of data to write to each of the path
+ * @param options Set the type of ZNode see the valid values in {@link
AccessOption}
+ * @param expectedSession The expected ZK session to create children
+ * @return For each child: true if creation succeeded, false otherwise (e.g.
if the child exists)
+ */
+ default boolean[] createChildren(List<String> paths, List<T> records, int
options,
Review comment:
After comparing options, I still think new interfaces would be easier.
We can have interfaces defined by extending the existing one, but have the
additional methods.
For example,
`public interface SessionAwareBaseDataAccessor<T> extends
BaseDataAccessor<T> {}`
The reason I prefer new interfaces is that the new methods do not make much
sense for most of the external users. And the parameters are confusing for
those who don't care about the session.
We shall try to keep the session aware accessors / clients as private as
possible.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageDispatchStage.java
##########
@@ -78,7 +78,17 @@ protected void processEvent(ClusterEvent event,
MessageOutput messageOutput) thr
batchMessage(dataAccessor.keyBuilder(), messagesToSend, resourceMap,
liveInstanceMap,
manager.getProperties());
- List<Message> messagesSent = sendMessages(dataAccessor, outputMessages);
+ String expectedSession =
event.getAttribute(AttributeName.EVENT_SESSION.name());
+ // An early check for expected leader session. If the sessions don't
match, it means the
+ // controller lost leadership, then messages should not be sent and the
pipeline is stopped.
+ // This potentially avoid double masters for a single partition.
+ if (expectedSession != null &&
!manager.getSessionId().equals(expectedSession)) {
Review comment:
nit, safer to go reverse way. Since you already checked expectedSession
is not null, so there is guarantee no NPE if you do expectedSession.equals(...)
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -401,6 +402,8 @@ void checkConnected(long timeout) {
throw new HelixException(
"HelixManager is not connected within retry timeout for cluster " +
_clusterName);
}
+
+ _sessionId = ZKUtil.toHexSessionId(_zkclient.getSessionId());
Review comment:
> If the sessionId changes before handleNewSession() is done, I expect
handleNewSession() should be able to handle the expired session because it is
handling an expected session
What if the session Id is changed before handleNewSession() get a chance to
execute? This will update the session Id unexpectedly. This means the handlers
are not correctly reset. I suggest not break this assumption.
On the other side, if we don't set this, then the operation will fail if
they are session aware. Isn't that a result that we want?
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1738,29 +1705,36 @@ public Stat writeDataGetStat(final String path, Object
datat, final int expected
return writeDataReturnStat(path, datat, expectedVersion);
}
- public void asyncCreate(final String path, Object datat, final CreateMode
mode,
Review comment:
Why changing the name? I think datat stands for data type. In this
scenario, it means this is an object with a certain data type. "data" is very
generic so it might be less meaningful.
I don't have a strong preference on the names. But if you are going to
change it, please change all the datat in this java file at least : ) Or even
better, the whole project.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -419,6 +420,17 @@ void
addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
Long getSessionStartTime();
+ /**
+ * Checks whether the cluster manager is leader and returns the session ID
associated to the
+ * connection of cluster data store, if and only if it is leader.
+ *
+ * @return {@code Optional<String>} session ID is present inside the {@code
Optional} object
+ * if the cluster manager is leader. Otherwise, returns an empty {@code
Optional} object.
+ */
+ default Optional<String> getSessionIdIfLead() {
Review comment:
Another concern is getSessionId() would become less useful with this
call. And would it be confusing for the caller which one to call?
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -2114,13 +2088,43 @@ 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) {
+ ZooKeeper zk = ((ZkConnection) getConnection()).getZookeeper();
+
+ if (expectedSessionId == null || expectedSessionId.isEmpty()) {
+ return zk;
+ }
+
+ /*
+ * 1. If operation is session aware, we have to check whether or not the
+ * passed-in(expected) session id matches actual session's id.
+ * If not, znode creation is failed. This validation is
+ * critical to guarantee the znode is created by the expected ZK session.
+ *
+ * 2. Otherwise, the operation is NOT session aware.
+ * In this case, we will use the actual zookeeper session to create the
node.
+ */
+ acquireEventLock();
+ try {
+ 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);
+ }
+
+ /*
+ * Cache the zookeeper reference and make sure later zooKeeper.create()
is being run
+ * under this zookeeper connection. This is to avoid locking
zooKeeper.create() which
+ * may cause potential performance issue.
+ */
+ return ((ZkConnection) getConnection()).getZookeeper();
Review comment:
return zk?
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -419,6 +420,17 @@ void
addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
Long getSessionStartTime();
+ /**
+ * Checks whether the cluster manager is leader and returns the session ID
associated to the
+ * connection of cluster data store, if and only if it is leader.
+ *
+ * @return {@code Optional<String>} session ID is present inside the {@code
Optional} object
+ * if the cluster manager is leader. Otherwise, returns an empty {@code
Optional} object.
+ */
+ default Optional<String> getSessionIdIfLead() {
Review comment:
It's good to consider backward compatibility here. But I believe
HeixManager is not a "public" interface, meaning we shall not assume the user
will implement this class and send back to any of the Helix components to use.
So I think we don't need to make it default and throws
UnsupportedOperationException.
@dasahcc and @lei-xia, what do you think?
##########
File path: helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java
##########
@@ -788,14 +788,22 @@ protected void setupInstances(String clusterName, int[]
instances) {
}
}
- protected void runPipeline(ClusterEvent event, Pipeline pipeline) {
+ protected void runPipeline(ClusterEvent event, Pipeline pipeline) throws
Exception {
Review comment:
Let's don't bother keeping the backward compatibility fo the test
classes.
My suggestion is that we throw the exception by default. But change the test
callers to catch it if necessary.
If this is not clean, then just add the additional parameters to the callers.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
##########
@@ -372,6 +372,12 @@ public void asyncCreate(String path, Object datat,
CreateMode mode,
_rawZkClient.asyncCreate(path, datat, mode, cb);
}
+ @Override
+ public void asyncCreate(String path, Object datat, CreateMode mode,
Review comment:
It's better to support it for the dedicated zkclient only. For the other
clients, especially customer-facing interface, I would prefer not to open this
API for now.
----------------------------------------------------------------
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]
