You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "xyuanlu (via GitHub)" <gi...@apache.org> on 2023/03/03 01:29:34 UTC
[GitHub] [helix] xyuanlu opened a new pull request, #2391: add support for state change
xyuanlu opened a new pull request, #2391:
URL: https://github.com/apache/helix/pull/2391
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
(Write a concise description including what, why, how)
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1130029231
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -65,11 +68,15 @@ enum ConnectState {
// Server has expired this connection.
EXPIRED,
- // When client failed to connect server.
- INIT_FAILED,
Review Comment:
INIT_FAILED will be covered in NOT_CONNECTED
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1130201796
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -275,7 +276,8 @@ public DirectChildSubscribeResult subscribeDirectChildChange(String key,
@Override
public boolean subscribeStateChanges(ConnectStateChangeListener listener) {
- return false;
+ _zkClient.subscribeStateChanges(new StateChangeListenerAdapter(listener));
+ return true;
Review Comment:
This is best effort with ZK implementation. We still want to keep the interface behavior.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1128718775
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -334,6 +320,36 @@ public void handleDirectChildChange(String key) throws Exception {
}
}
+ @Test
+ public void testConnectStateChangeListener() throws Exception {
+ final String basePath = "/TestZkMetaClient_testConnectionStateChangeListener";
+ try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
+ CountDownLatch countDownLatch = new CountDownLatch(1);
+ final MetaClientInterface.ConnectState[] connectState =
+ new MetaClientInterface.ConnectState[2];
+ ConnectStateChangeListener listener = new ConnectStateChangeListener() {
+ @Override
+ public void handleConnectStateChanged(MetaClientInterface.ConnectState prevState,
+ MetaClientInterface.ConnectState currentState) throws Exception {
+ System.out.println("from: " + prevState + " to State: " + currentState);
Review Comment:
TFTR. Removed debugging prints.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rahulrane50 commented on a diff in pull request #2391: add support for state change
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1129910557
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -65,11 +68,15 @@ enum ConnectState {
// Server has expired this connection.
EXPIRED,
- // When client failed to connect server.
- INIT_FAILED,
-
// When client explicitly call disconnect.
- CLOSED_BY_CLIENT
+ CLOSED_BY_CLIENT,
+
+ // Connection between client and server is lost.
+ DISCONNECTED,
+
+ // Client is authenticated. They can perform operation with authorized permissions.
+ // This state is not in use as of now.
+ AUTHENTICATED
Review Comment:
IMHO we should add neither "AUTHENTICATED" nor "AUTH_FAILED" because that's not "connection state" but just return code of connection request but upto i don't have any strong objection on this.
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -65,11 +68,15 @@ enum ConnectState {
// Server has expired this connection.
EXPIRED,
- // When client failed to connect server.
- INIT_FAILED,
Review Comment:
Curious, why did we remove this?
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -275,7 +276,8 @@ public DirectChildSubscribeResult subscribeDirectChildChange(String key,
@Override
public boolean subscribeStateChanges(ConnectStateChangeListener listener) {
- return false;
+ _zkClient.subscribeStateChanges(new StateChangeListenerAdapter(listener));
+ return true;
Review Comment:
If returning true means successful subscription then we should update it's documentation correctly that "it would either return true in case of success or raise an exception" here or in MetaClientInterface.java
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/IZkStateListener.java:
##########
@@ -58,4 +58,8 @@ public interface IZkStateListener {
* On any error.
*/
void handleSessionEstablishmentError(final Throwable error) throws Exception;
+
+ default void handleStateChanged(KeeperState prevState, KeeperState curState) throws Exception {
Review Comment:
I think intention here is to have backward compatibility with old API until we move to new API which accepts both args. It would be good to have this added in comments.
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -334,6 +320,35 @@ public void handleDirectChildChange(String key) throws Exception {
}
}
+ @Test
+ public void testConnectStateChangeListener() throws Exception {
Review Comment:
neat : good to add few comments above this test what all test scenarios it's covering. Once it covers all scenarios it's obvious so no comment is needed but until then reader might not understand what is covered and what is left.
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/util/ZkMetaClientUtil.java:
##########
@@ -239,6 +240,29 @@ public static MetaClientException translateZkExceptionToMetaclientException(ZkEx
return new MetaClientException(e);
}
+ public static MetaClientInterface.ConnectState translateKeeperStateToMetaClientConnectState(
+ Watcher.Event.KeeperState keeperState) {
+ if (keeperState == null)
+ return MetaClientInterface.ConnectState.NOT_CONNECTED;
+ switch (keeperState) {
+ case AuthFailed:
+ return MetaClientInterface.ConnectState.AUTH_FAILED;
+ case Closed:
+ return MetaClientInterface.ConnectState.CLOSED_BY_CLIENT;
+ case Disconnected:
+ return MetaClientInterface.ConnectState.DISCONNECTED;
+ case Expired:
+ return MetaClientInterface.ConnectState.EXPIRED;
+ case SaslAuthenticated:
Review Comment:
neat : this is not entirely true. RIght now we don't use ZK SASL authentication so you will always see Sasl auth failure. But still it's not "auth failed" case because we do x509 authentication. Right not it's fine but may be you can add TODO to fix this when we want to onboard on ZK ACL.
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1485,12 +1485,13 @@ private Stat installWatchOnlyPathExist(final String path) {
protected void processStateChanged(WatchedEvent event) {
LOG.info("zkclient {}, zookeeper state changed ( {} )", _uid, event.getState());
+ KeeperState prevState = _currentState;
setCurrentState(event.getState());
if (getShutdownTrigger()) {
return;
}
- fireStateChangedEvent(event.getState());
+ fireStateChangedEvent(prevState, event.getState());
Review Comment:
neat : we can directly use _currentState here right?
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -300,7 +302,7 @@ public void unsubscribeChildChanges(String key, ChildChangeListener listener) {
@Override
public void unsubscribeConnectStateChanges(ConnectStateChangeListener listener) {
-
+ _zkClient.subscribeStateChanges(new StateChangeListenerAdapter(listener));
Review Comment:
Just noticed that we don't return any error or return code if unsubscribe fails. I see this comment "No-op if the listener is not subscribed to the key.". Does it mean that caller has to call unsubscribe again if it fails? and if that's the case then how that can be communicated to user?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1129946354
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/util/ZkMetaClientUtil.java:
##########
@@ -239,6 +240,29 @@ public static MetaClientException translateZkExceptionToMetaclientException(ZkEx
return new MetaClientException(e);
}
+ public static MetaClientInterface.ConnectState translateKeeperStateToMetaClientConnectState(
+ Watcher.Event.KeeperState keeperState) {
+ if (keeperState == null)
+ return MetaClientInterface.ConnectState.NOT_CONNECTED;
+ switch (keeperState) {
+ case AuthFailed:
+ return MetaClientInterface.ConnectState.AUTH_FAILED;
+ case Closed:
+ return MetaClientInterface.ConnectState.CLOSED_BY_CLIENT;
+ case Disconnected:
+ return MetaClientInterface.ConnectState.DISCONNECTED;
+ case Expired:
+ return MetaClientInterface.ConnectState.EXPIRED;
+ case SaslAuthenticated:
Review Comment:
When we dont use ZK SASL authentication, will Zk Server return auth failed?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] qqu0127 commented on a diff in pull request #2391: add support for state change
Posted by "qqu0127 (via GitHub)" <gi...@apache.org>.
qqu0127 commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1128187083
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/adapter/StateChangeAdapter.java:
##########
@@ -0,0 +1,75 @@
+package org.apache.helix.metaclient.impl.zk.adapter;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.helix.metaclient.api.ConnectStateChangeListener;
+import org.apache.helix.metaclient.exception.MetaClientTimeoutException;
+import org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil;
+import org.apache.helix.zookeeper.zkclient.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkInterruptedException;
+import org.apache.zookeeper.Watcher;
+
+
+public class StateChangeAdapter implements IZkStateListener {
+ private final ConnectStateChangeListener _listener;
+
+ public StateChangeAdapter(ConnectStateChangeListener listener) {
+ _listener = listener;
+ }
+
+ @Override
+ public void handleStateChanged(Watcher.Event.KeeperState state) throws Exception {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void handleNewSession(String sessionId) throws Exception {
+ }
Review Comment:
Should we throw `UnsupportedOperationException` for this one too?
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -334,6 +320,36 @@ public void handleDirectChildChange(String key) throws Exception {
}
}
+ @Test
+ public void testConnectStateChangeListener() throws Exception {
+ final String basePath = "/TestZkMetaClient_testConnectionStateChangeListener";
+ try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
+ CountDownLatch countDownLatch = new CountDownLatch(1);
+ final MetaClientInterface.ConnectState[] connectState =
+ new MetaClientInterface.ConnectState[2];
+ ConnectStateChangeListener listener = new ConnectStateChangeListener() {
+ @Override
+ public void handleConnectStateChanged(MetaClientInterface.ConnectState prevState,
+ MetaClientInterface.ConnectState currentState) throws Exception {
+ System.out.println("from: " + prevState + " to State: " + currentState);
Review Comment:
Is this still needed?
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -334,6 +320,36 @@ public void handleDirectChildChange(String key) throws Exception {
}
}
+ @Test
+ public void testConnectStateChangeListener() throws Exception {
+ final String basePath = "/TestZkMetaClient_testConnectionStateChangeListener";
+ try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
+ CountDownLatch countDownLatch = new CountDownLatch(1);
+ final MetaClientInterface.ConnectState[] connectState =
+ new MetaClientInterface.ConnectState[2];
+ ConnectStateChangeListener listener = new ConnectStateChangeListener() {
+ @Override
+ public void handleConnectStateChanged(MetaClientInterface.ConnectState prevState,
+ MetaClientInterface.ConnectState currentState) throws Exception {
+ System.out.println("from: " + prevState + " to State: " + currentState);
+ connectState[0] = prevState;
+ connectState[1] = currentState;
+ countDownLatch.countDown();
+ }
+
+ @Override
+ public void handleConnectionEstablishmentError(Throwable error) throws Exception {
+
+ }
+ };
+ Assert.assertTrue(zkMetaClient.subscribeStateChanges(listener));
+ zkMetaClient.connect();
+ countDownLatch.await(5000, TimeUnit.SECONDS);
+ Assert.assertEquals(connectState[0], MetaClientInterface.ConnectState.NOT_CONNECTED);
+ Assert.assertEquals(connectState[1], MetaClientInterface.ConnectState.CONNECTED);
+ }
Review Comment:
Nice to have: can we have more test cases on other state changes?
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/adapter/StateChangeAdapter.java:
##########
@@ -0,0 +1,75 @@
+package org.apache.helix.metaclient.impl.zk.adapter;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.helix.metaclient.api.ConnectStateChangeListener;
+import org.apache.helix.metaclient.exception.MetaClientTimeoutException;
+import org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil;
+import org.apache.helix.zookeeper.zkclient.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkInterruptedException;
+import org.apache.zookeeper.Watcher;
+
+
+public class StateChangeAdapter implements IZkStateListener {
Review Comment:
Not an issue, StateChangeListenerAdapter might be more accurate? (but more verbose)
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rahulrane50 commented on a diff in pull request #2391: add support for state change
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1131464312
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/util/ZkMetaClientUtil.java:
##########
@@ -239,6 +240,29 @@ public static MetaClientException translateZkExceptionToMetaclientException(ZkEx
return new MetaClientException(e);
}
+ public static MetaClientInterface.ConnectState translateKeeperStateToMetaClientConnectState(
+ Watcher.Event.KeeperState keeperState) {
+ if (keeperState == null)
+ return MetaClientInterface.ConnectState.NOT_CONNECTED;
+ switch (keeperState) {
+ case AuthFailed:
+ return MetaClientInterface.ConnectState.AUTH_FAILED;
+ case Closed:
+ return MetaClientInterface.ConnectState.CLOSED_BY_CLIENT;
+ case Disconnected:
+ return MetaClientInterface.ConnectState.DISCONNECTED;
+ case Expired:
+ return MetaClientInterface.ConnectState.EXPIRED;
+ case SaslAuthenticated:
Review Comment:
Whenever x509/any other authentication fails then zk server will return auth failed.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1129947026
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1485,12 +1485,13 @@ private Stat installWatchOnlyPathExist(final String path) {
protected void processStateChanged(WatchedEvent event) {
LOG.info("zkclient {}, zookeeper state changed ( {} )", _uid, event.getState());
+ KeeperState prevState = _currentState;
setCurrentState(event.getState());
if (getShutdownTrigger()) {
return;
}
- fireStateChangedEvent(event.getState());
+ fireStateChangedEvent(prevState, event.getState());
Review Comment:
No. _currentState is updated in line 1489.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1128718544
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -334,6 +320,36 @@ public void handleDirectChildChange(String key) throws Exception {
}
}
+ @Test
+ public void testConnectStateChangeListener() throws Exception {
+ final String basePath = "/TestZkMetaClient_testConnectionStateChangeListener";
+ try (ZkMetaClient<String> zkMetaClient = createZkMetaClient()) {
+ CountDownLatch countDownLatch = new CountDownLatch(1);
+ final MetaClientInterface.ConnectState[] connectState =
+ new MetaClientInterface.ConnectState[2];
+ ConnectStateChangeListener listener = new ConnectStateChangeListener() {
+ @Override
+ public void handleConnectStateChanged(MetaClientInterface.ConnectState prevState,
+ MetaClientInterface.ConnectState currentState) throws Exception {
+ System.out.println("from: " + prevState + " to State: " + currentState);
+ connectState[0] = prevState;
+ connectState[1] = currentState;
+ countDownLatch.countDown();
+ }
+
+ @Override
+ public void handleConnectionEstablishmentError(Throwable error) throws Exception {
+
+ }
+ };
+ Assert.assertTrue(zkMetaClient.subscribeStateChanges(listener));
+ zkMetaClient.connect();
+ countDownLatch.await(5000, TimeUnit.SECONDS);
+ Assert.assertEquals(connectState[0], MetaClientInterface.ConnectState.NOT_CONNECTED);
+ Assert.assertEquals(connectState[1], MetaClientInterface.ConnectState.CONNECTED);
+ }
Review Comment:
Will add more test case when connection lost and reconnect is implemented.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1130212068
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -65,11 +68,15 @@ enum ConnectState {
// Server has expired this connection.
EXPIRED,
- // When client failed to connect server.
- INIT_FAILED,
-
// When client explicitly call disconnect.
- CLOSED_BY_CLIENT
+ CLOSED_BY_CLIENT,
+
+ // Connection between client and server is lost.
+ DISCONNECTED,
+
+ // Client is authenticated. They can perform operation with authorized permissions.
+ // This state is not in use as of now.
+ AUTHENTICATED
Review Comment:
The connection state represent the connection state change between client and metadata service. Listener will get notification when state change. This is not used to determine the current connection state of the client.
ZK will send event for `AUTHENTICATED` as connection event change. I don't think we should swallow it ...
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1128719656
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/adapter/StateChangeAdapter.java:
##########
@@ -0,0 +1,75 @@
+package org.apache.helix.metaclient.impl.zk.adapter;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.helix.metaclient.api.ConnectStateChangeListener;
+import org.apache.helix.metaclient.exception.MetaClientTimeoutException;
+import org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil;
+import org.apache.helix.zookeeper.zkclient.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkInterruptedException;
+import org.apache.zookeeper.Watcher;
+
+
+public class StateChangeAdapter implements IZkStateListener {
+ private final ConnectStateChangeListener _listener;
+
+ public StateChangeAdapter(ConnectStateChangeListener listener) {
+ _listener = listener;
+ }
+
+ @Override
+ public void handleStateChanged(Watcher.Event.KeeperState state) throws Exception {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void handleNewSession(String sessionId) throws Exception {
+ }
Review Comment:
handleNewSession will be invoked when new connection is established. This event is covered in another callback. Will not expose this to user.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1128718196
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/adapter/StateChangeAdapter.java:
##########
@@ -0,0 +1,75 @@
+package org.apache.helix.metaclient.impl.zk.adapter;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.helix.metaclient.api.ConnectStateChangeListener;
+import org.apache.helix.metaclient.exception.MetaClientTimeoutException;
+import org.apache.helix.metaclient.impl.zk.util.ZkMetaClientUtil;
+import org.apache.helix.zookeeper.zkclient.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkInterruptedException;
+import org.apache.zookeeper.Watcher;
+
+
+public class StateChangeAdapter implements IZkStateListener {
Review Comment:
TFTR. Updated.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1130201283
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -300,7 +302,7 @@ public void unsubscribeChildChanges(String key, ChildChangeListener listener) {
@Override
public void unsubscribeConnectStateChanges(ConnectStateChangeListener listener) {
-
+ _zkClient.subscribeStateChanges(new StateChangeListenerAdapter(listener));
Review Comment:
Runtime Exception will be thrown if ZkClient unsubscribe failed.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1130202202
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -334,6 +320,35 @@ public void handleDirectChildChange(String key) throws Exception {
}
}
+ @Test
+ public void testConnectStateChangeListener() throws Exception {
Review Comment:
Next PR will cover other stages. Won't have gap in between releases.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rahulrane50 commented on a diff in pull request #2391: add support for state change
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1131464312
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/util/ZkMetaClientUtil.java:
##########
@@ -239,6 +240,29 @@ public static MetaClientException translateZkExceptionToMetaclientException(ZkEx
return new MetaClientException(e);
}
+ public static MetaClientInterface.ConnectState translateKeeperStateToMetaClientConnectState(
+ Watcher.Event.KeeperState keeperState) {
+ if (keeperState == null)
+ return MetaClientInterface.ConnectState.NOT_CONNECTED;
+ switch (keeperState) {
+ case AuthFailed:
+ return MetaClientInterface.ConnectState.AUTH_FAILED;
+ case Closed:
+ return MetaClientInterface.ConnectState.CLOSED_BY_CLIENT;
+ case Disconnected:
+ return MetaClientInterface.ConnectState.DISCONNECTED;
+ case Expired:
+ return MetaClientInterface.ConnectState.EXPIRED;
+ case SaslAuthenticated:
Review Comment:
we never use sasl authentication and hence you will always see error log about "sasl auth failure." Whenever x509/any other authentication fails then zk server will return auth failed.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rahulrane50 commented on a diff in pull request #2391: add support for state change
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1131536460
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/util/ZkMetaClientUtil.java:
##########
@@ -239,6 +240,29 @@ public static MetaClientException translateZkExceptionToMetaclientException(ZkEx
return new MetaClientException(e);
}
+ public static MetaClientInterface.ConnectState translateKeeperStateToMetaClientConnectState(
+ Watcher.Event.KeeperState keeperState) {
+ if (keeperState == null)
+ return MetaClientInterface.ConnectState.NOT_CONNECTED;
+ switch (keeperState) {
+ case AuthFailed:
+ return MetaClientInterface.ConnectState.AUTH_FAILED;
+ case Closed:
+ return MetaClientInterface.ConnectState.CLOSED_BY_CLIENT;
+ case Disconnected:
+ return MetaClientInterface.ConnectState.DISCONNECTED;
+ case Expired:
+ return MetaClientInterface.ConnectState.EXPIRED;
+ case SaslAuthenticated:
Review Comment:
Had a discussion offline, for any authentication scheme being used by underlying zk client, it should return "AUTHENTICATED->CONNECTED" events in this order and in case of auth failure it should be "AUTH_FAILED" event. Now if it doesn't use SASL then it should not return AUTH_FAILED event.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a diff in pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2391:
URL: https://github.com/apache/helix/pull/2391#discussion_r1130071466
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/IZkStateListener.java:
##########
@@ -58,4 +58,8 @@ public interface IZkStateListener {
* On any error.
*/
void handleSessionEstablishmentError(final Throwable error) throws Exception;
+
+ default void handleStateChanged(KeeperState prevState, KeeperState curState) throws Exception {
Review Comment:
Not essentially. The intention here is how to use current ZkClient in ZKMetaClient implementation.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #2391:
URL: https://github.com/apache/helix/pull/2391#issuecomment-1463006521
This change is ready to be merged. Approved by @qqu0127
Commit message: Add support for state change in ZkMetaClient
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu merged pull request #2391: add support for state change
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2391:
URL: https://github.com/apache/helix/pull/2391
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org