You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "Marcosrico (via GitHub)" <gi...@apache.org> on 2023/02/14 21:58:39 UTC
[GitHub] [helix] Marcosrico opened a new pull request, #2377: MetaClient Container Node Implementation
Marcosrico opened a new pull request, #2377:
URL: https://github.com/apache/helix/pull/2377
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
https://github.com/apache/helix/issues/2237
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
Implementation of Container Node in ZkMetaClient.
### Tests
- [x] The following tests are written for this issue:
Unit test `createContainer()`.
Note: Cannot test end to end behavior (i.e. that the container node disappears after it's children are deleted) as the current setup of the zk server doesn't permit it. This is also a issue in HelixZkClient testbase. However, it has been tested and confirmed that the actual behavior does work (in native zkclient) and is merely a unit testing bug. Will track and fix this effort in the future but current implementation will use existing test base framework.
- 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.)
### 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 merged pull request #2377: MetaClient Container Node Implementation
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2377:
URL: https://github.com/apache/helix/pull/2377
--
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 #2377: MetaClient Container Node Implementation
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2377:
URL: https://github.com/apache/helix/pull/2377#discussion_r1106452843
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -67,8 +68,16 @@ public class TestZkMetaClient {
private ZkServer _zkServer;
+ /**
+ * Creates local Zk Server
+ * Note: Cannot test container / TTL node end to end behavior as
+ * the zk server setup doesn't allow for that. However, the actual
+ * behavior has been verified to work on native ZK Client.
Review Comment:
Thanks for the change. Could you please give a bit more detail explaining why the set up dues not support container / TTL node and what are needed if we want to support that in unit test?
--
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 #2377: MetaClient Container Node Implementation
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2377:
URL: https://github.com/apache/helix/pull/2377#discussion_r1107621958
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -67,8 +68,16 @@ public class TestZkMetaClient {
private ZkServer _zkServer;
+ /**
+ * Creates local Zk Server
+ * Note: Cannot test container / TTL node end to end behavior as
+ * the zk server setup doesn't allow for that. However, the actual
+ * behavior has been verified to work on native ZK Client.
Review Comment:
+1 to ZooKeeperServerMain. Sometime back while debugging some issue in ZooInspector i noticed this but it's bad that i didn't raise it up back then. Thanks Marcos! Your explanation was very insightful!
--
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] Marcosrico commented on pull request #2377: MetaClient Container Node Implementation
Posted by "Marcosrico (via GitHub)" <gi...@apache.org>.
Marcosrico commented on PR #2377:
URL: https://github.com/apache/helix/pull/2377#issuecomment-1431815742
This PR is ready to merge, approved by @qqu0127
Commit message:
Implementation of Container Nodes 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] Marcosrico commented on a diff in pull request #2377: MetaClient Container Node Implementation
Posted by "Marcosrico (via GitHub)" <gi...@apache.org>.
Marcosrico commented on code in PR #2377:
URL: https://github.com/apache/helix/pull/2377#discussion_r1107315244
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -67,8 +68,16 @@ public class TestZkMetaClient {
private ZkServer _zkServer;
+ /**
+ * Creates local Zk Server
+ * Note: Cannot test container / TTL node end to end behavior as
+ * the zk server setup doesn't allow for that. However, the actual
+ * behavior has been verified to work on native ZK Client.
Review Comment:
To enable proper end-to-end behavior between TTL and Container nodes in unit testing, it is necessary to invoke ContainerManager.java when setting up the Zk server. ContainerManager object is responsible for managing the creation / deletion of znodes (lifecycle of the nodes).
Since nodes with TTL and Container modes are designed to be deleted after a certain interval, a zk server with a ContainerManager is required for running these nodes. This can be achieved by using classes such as LeaderZooKeeperServer.java or ZooKeeperServerMain.java. However, the current implementation of zk server in metaclient and helix test base utilizes a simplified zk server setup (ZooKeeperServer), which lacks a ContainerManager implementation.
It is necessary to create both a ContainerManager and `System.setProperty("zookeeper.extendedTypesEnabled", "true");` in order for TTL and Container nodes to function properly.
For future work, the setup of the zk server needs to be changed to use the appropriate classes mentioned above to enable the proper functioning of TTL and Container nodes in unit testing. I'll update the comments.
--
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 #2377: MetaClient Container Node Implementation
Posted by "qqu0127 (via GitHub)" <gi...@apache.org>.
qqu0127 commented on code in PR #2377:
URL: https://github.com/apache/helix/pull/2377#discussion_r1107358848
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -67,8 +68,16 @@ public class TestZkMetaClient {
private ZkServer _zkServer;
+ /**
+ * Creates local Zk Server
+ * Note: Cannot test container / TTL node end to end behavior as
+ * the zk server setup doesn't allow for that. However, the actual
+ * behavior has been verified to work on native ZK Client.
Review Comment:
Thanks for the info Marcos!
Is this a Zookeeper side issue for the inconsistency? Do we need ZK version upgrade to have ContainerManager? Just want to understand if we have external dependency in order to test this.
--
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] Marcosrico commented on a diff in pull request #2377: MetaClient Container Node Implementation
Posted by "Marcosrico (via GitHub)" <gi...@apache.org>.
Marcosrico commented on code in PR #2377:
URL: https://github.com/apache/helix/pull/2377#discussion_r1107526612
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -67,8 +68,16 @@ public class TestZkMetaClient {
private ZkServer _zkServer;
+ /**
+ * Creates local Zk Server
+ * Note: Cannot test container / TTL node end to end behavior as
+ * the zk server setup doesn't allow for that. However, the actual
+ * behavior has been verified to work on native ZK Client.
Review Comment:
Of course! And no to both. I believe the `ZooKeeperServer `class exists in Zk just to create servers easier but with less functionalities. And we do not need to upgrade any ZK version as the classes mentioned above are available in the current version of zk that we use.
Just for additional information, I was able to successfully create a zk server using `ZooKeeperServerMain.java,` but replacing the existing framework and modifying it so it's thread safe would require more bandwidth than available. And it is not a major issue so the priority is low.
--
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] Marcosrico commented on a diff in pull request #2377: MetaClient Container Node Implementation
Posted by "Marcosrico (via GitHub)" <gi...@apache.org>.
Marcosrico commented on code in PR #2377:
URL: https://github.com/apache/helix/pull/2377#discussion_r1107315244
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -67,8 +68,16 @@ public class TestZkMetaClient {
private ZkServer _zkServer;
+ /**
+ * Creates local Zk Server
+ * Note: Cannot test container / TTL node end to end behavior as
+ * the zk server setup doesn't allow for that. However, the actual
+ * behavior has been verified to work on native ZK Client.
Review Comment:
To enable proper end-to-end behavior between TTL and Container nodes in unit testing, it is necessary to invoke ContainerManager.java when setting up the Zk server. ContainerManager object is responsible for managing the creation / deletion of znodes (lifecycle of the nodes).
Since nodes with TTL and Container modes are designed to be deleted after a certain interval, a zk server with a ContainerManager is required for running these nodes. This can be achieved by using classes such as LeaderZooKeeperServer.java or ZooKeeperServerMain.java. However, the current implementation of zk server in metaclient and helix test base utilizes a simplified version of ZooKeeperServer, which lacks a ContainerManager implementation.
It is necessary to create both a ContainerManager and `System.setProperty("zookeeper.extendedTypesEnabled", "true");` in order for TTL and Container nodes to function properly.
For future work, the setup of the zk server needs to be changed to use the appropriate classes mentioned above to enable the proper functioning of TTL and Container nodes in unit testing. I'll update the comments.
--
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] mgao0 commented on a diff in pull request #2377: MetaClient Container Node Implementation
Posted by "mgao0 (via GitHub)" <gi...@apache.org>.
mgao0 commented on code in PR #2377:
URL: https://github.com/apache/helix/pull/2377#discussion_r1106542090
##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -67,8 +68,16 @@ public class TestZkMetaClient {
private ZkServer _zkServer;
+ /**
+ * Creates local Zk Server
+ * Note: Cannot test container / TTL node end to end behavior as
+ * the zk server setup doesn't allow for that. However, the actual
+ * behavior has been verified to work on native ZK Client.
Review Comment:
+1
Does setting this `System.setProperty("zookeeper.extendedTypesEnabled", "true");` not help?
--
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