You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/06/03 02:50:05 UTC
[GitHub] [helix] pkuwm opened a new pull request #1053: Fix test testDropInstance
pkuwm opened a new pull request #1053:
URL: https://github.com/apache/helix/pull/1053
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1051
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Test testDropInstance doesn't assert expected exception and ignores the unexpected behavior. ZkBaseDataAccessor is created using a shared zkclient to create a live instance. The creation fails because shared zkclient doesn't support creating a live instance.
This PR addresses this problem by creating a dedicated zkclient in ZkBaseDataAccessor to create a live instance.
### Tests
- [ ] The following tests are written for this issue:
N/A
- [ ] The following is the result of the "mvn test" command on the appropriate module:
### 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"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### 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.
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] pkuwm commented on a change in pull request #1053: Fix test testDropInstance
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1053:
URL: https://github.com/apache/helix/pull/1053#discussion_r434794029
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -356,51 +360,65 @@ public void testDropInstance() throws Exception {
"MasterSlave", true); // do rebalance
// add fake liveInstance
- ZKHelixDataAccessor accessor =
- new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(ZK_ADDR));
Review comment:
@lei-xia No, it doesn't. Before 1.0.0, a ZKHelixDataAccessor with a ZK-ADDR is not able create an ephemeral node. It is also by default using a shared zkclient internally. The only difference is the type of exceptions: before 1.0.0, `HelixException` is thrown from sharedZkClient, while in 1.0.0, `UnsupportedOperationException` is thrown.
The root cause for this test is, even though a live instance(ephemeral node) is not created by the ZKHelixDataAccessor with a ZK-ADDR, the test still passes, because the test doesn't catch or verify the correct result.
----------------------------------------------------------------
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:
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] pkuwm commented on pull request #1053: Fix test testDropInstance
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1053:
URL: https://github.com/apache/helix/pull/1053#issuecomment-637938685
This PR is ready to be merged, approved by @narendly
```
Test testDropInstance doesn't assert expected HelixException, so when a live instance is not created, the test still passes. The reason is the test uses a sharedZkClient to create a live instance but fails.
This PR addresses this issue by creating a dedicated zkclient in ZkBaseDataAccessor to create a live instance.
```
----------------------------------------------------------------
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:
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] pkuwm commented on a change in pull request #1053: Fix test testDropInstance
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1053:
URL: https://github.com/apache/helix/pull/1053#discussion_r434794029
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -356,51 +360,65 @@ public void testDropInstance() throws Exception {
"MasterSlave", true); // do rebalance
// add fake liveInstance
- ZKHelixDataAccessor accessor =
- new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(ZK_ADDR));
Review comment:
@lei-xia No, it doesn't. Before 1.0.0, a ZKHelixDataAccessor with a ZK-ADDR is not able to create an ephemeral node, either. It is also by default using a shared zkclient internally. The only difference is the type of exceptions: before 1.0.0, `HelixException` is thrown from sharedZkClient, while in 1.0.0, `UnsupportedOperationException` is thrown.
The root cause for this test is, even though a live instance(ephemeral node) is not created by the ZKHelixDataAccessor with a ZK-ADDR, the test still passes, because the test doesn't catch or verify the correct result.
----------------------------------------------------------------
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:
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] narendly merged pull request #1053: Fix test testDropInstance
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #1053:
URL: https://github.com/apache/helix/pull/1053
----------------------------------------------------------------
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:
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] lei-xia commented on a change in pull request #1053: Fix test testDropInstance
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #1053:
URL: https://github.com/apache/helix/pull/1053#discussion_r434663662
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -356,51 +360,65 @@ public void testDropInstance() throws Exception {
"MasterSlave", true); // do rebalance
// add fake liveInstance
- ZKHelixDataAccessor accessor =
- new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(ZK_ADDR));
Review comment:
Does this mean if user constructs a ZKHelixDataAccessor with a ZK-ADDR, they can create an ephemeral node before, but not anymore after they bump to 1.0.0?
----------------------------------------------------------------
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:
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] narendly commented on a change in pull request #1053: Fix test testDropInstance
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1053:
URL: https://github.com/apache/helix/pull/1053#discussion_r434287996
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -356,51 +360,57 @@ public void testDropInstance() throws Exception {
"MasterSlave", true); // do rebalance
// add fake liveInstance
- ZKHelixDataAccessor accessor =
- new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(ZK_ADDR));
- Builder keyBuilder = new Builder(clusterName);
- LiveInstance liveInstance = new LiveInstance("localhost_12918");
- liveInstance.setSessionId("session_0");
- liveInstance.setHelixVersion("version_0");
- accessor.setProperty(keyBuilder.liveInstance("localhost_12918"), liveInstance);
+ HelixDataAccessor accessor = new ZKHelixDataAccessor(clusterName,
+ new ZkBaseDataAccessor.Builder<ZNRecord>()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkClientType(ZkBaseDataAccessor.ZkClientType.DEDICATED)
+ .setZkAddress(ZK_ADDR)
+ .build());
- // drop without stop the process, should throw exception
try {
- ClusterSetup.processCommandLineArgs(new String[] {
- "--zkSvr", ZK_ADDR, "--dropNode", clusterName, "localhost:12918"
- });
- Assert.fail("Should throw exception since localhost_12918 is still in LIVEINSTANCES/");
- } catch (Exception e) {
- // OK
+ Builder keyBuilder = new Builder(clusterName);
+ LiveInstance liveInstance = new LiveInstance(instanceName);
+ liveInstance.setSessionId("session_0");
+ liveInstance.setHelixVersion("version_0");
+ accessor.setProperty(keyBuilder.liveInstance(instanceName), liveInstance);
+
+ // Drop instance without stopping the live instance, should throw HelixException
+ try {
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--dropNode", clusterName, instanceAddress});
+ Assert.fail("Should throw exception since localhost_12918 is still in LIVEINSTANCES/");
+ } catch (HelixException expected) {
+ Assert.assertEquals(expected.getMessage(),
+ "Cannot drop instance " + instanceName + " as it is still live. Please stop it first");
+ }
+ accessor.removeProperty(keyBuilder.liveInstance(instanceName));
+
+ // drop without disable, should throw exception
+ try {
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--dropNode", clusterName, instanceAddress});
+ Assert.fail("Should throw exception since " + instanceName + " is enabled");
+ } catch (HelixException expected) {
+ Assert.assertEquals(expected.getMessage(),
+ "Node " + instanceName + " is enabled, cannot drop");
+ }
+
+ // Disable the instance
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--enableInstance", clusterName, instanceName, "false"});
+ // Drop the instance
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--dropNode", clusterName, instanceAddress});
+
+ Assert.assertNull(accessor.getProperty(keyBuilder.instanceConfig(instanceName)),
+ "Instance config should be dropped");
+ Assert.assertFalse(_gZkClient.exists(PropertyPathBuilder.instance(clusterName, instanceName)),
+ "Instance/host should be dropped");
+ } finally {
+ // Have to close the dedicated zkclient in accessor to avoid zkclient leakage.
+ accessor.getBaseDataAccessor().close();
+ TestHelper.dropCluster(clusterName, _gZkClient);
Review comment:
Nit: usually a good idea to add a TestHelper.verify() to make sure the cluster has been dropped. This is recommended.
----------------------------------------------------------------
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:
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] pkuwm commented on a change in pull request #1053: Fix test testDropInstance
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1053:
URL: https://github.com/apache/helix/pull/1053#discussion_r434294418
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -356,51 +360,57 @@ public void testDropInstance() throws Exception {
"MasterSlave", true); // do rebalance
// add fake liveInstance
- ZKHelixDataAccessor accessor =
- new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(ZK_ADDR));
- Builder keyBuilder = new Builder(clusterName);
- LiveInstance liveInstance = new LiveInstance("localhost_12918");
- liveInstance.setSessionId("session_0");
- liveInstance.setHelixVersion("version_0");
- accessor.setProperty(keyBuilder.liveInstance("localhost_12918"), liveInstance);
+ HelixDataAccessor accessor = new ZKHelixDataAccessor(clusterName,
+ new ZkBaseDataAccessor.Builder<ZNRecord>()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkClientType(ZkBaseDataAccessor.ZkClientType.DEDICATED)
+ .setZkAddress(ZK_ADDR)
+ .build());
- // drop without stop the process, should throw exception
try {
- ClusterSetup.processCommandLineArgs(new String[] {
- "--zkSvr", ZK_ADDR, "--dropNode", clusterName, "localhost:12918"
- });
- Assert.fail("Should throw exception since localhost_12918 is still in LIVEINSTANCES/");
- } catch (Exception e) {
- // OK
+ Builder keyBuilder = new Builder(clusterName);
+ LiveInstance liveInstance = new LiveInstance(instanceName);
+ liveInstance.setSessionId("session_0");
+ liveInstance.setHelixVersion("version_0");
+ accessor.setProperty(keyBuilder.liveInstance(instanceName), liveInstance);
+
+ // Drop instance without stopping the live instance, should throw HelixException
+ try {
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--dropNode", clusterName, instanceAddress});
+ Assert.fail("Should throw exception since localhost_12918 is still in LIVEINSTANCES/");
+ } catch (HelixException expected) {
+ Assert.assertEquals(expected.getMessage(),
+ "Cannot drop instance " + instanceName + " as it is still live. Please stop it first");
+ }
+ accessor.removeProperty(keyBuilder.liveInstance(instanceName));
+
+ // drop without disable, should throw exception
+ try {
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--dropNode", clusterName, instanceAddress});
+ Assert.fail("Should throw exception since " + instanceName + " is enabled");
+ } catch (HelixException expected) {
+ Assert.assertEquals(expected.getMessage(),
+ "Node " + instanceName + " is enabled, cannot drop");
+ }
+
+ // Disable the instance
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--enableInstance", clusterName, instanceName, "false"});
+ // Drop the instance
+ ClusterSetup.processCommandLineArgs(
+ new String[]{"--zkSvr", ZK_ADDR, "--dropNode", clusterName, instanceAddress});
+
+ Assert.assertNull(accessor.getProperty(keyBuilder.instanceConfig(instanceName)),
+ "Instance config should be dropped");
+ Assert.assertFalse(_gZkClient.exists(PropertyPathBuilder.instance(clusterName, instanceName)),
+ "Instance/host should be dropped");
+ } finally {
+ // Have to close the dedicated zkclient in accessor to avoid zkclient leakage.
+ accessor.getBaseDataAccessor().close();
+ TestHelper.dropCluster(clusterName, _gZkClient);
Review comment:
Verified and 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.
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