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/05/13 21:52:00 UTC
[GitHub] [helix] narendly commented on a change in pull request #1007: Add more accurate error message for resetPartition
narendly commented on a change in pull request #1007:
URL: https://github.com/apache/helix/pull/1007#discussion_r424749831
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,9 +573,17 @@ public void resetPartition(String clusterName, String instanceName, String resou
// check the instance is alive
LiveInstance liveInstance = accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + " on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has ever exist in the cluster
Review comment:
"has ever existed"
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,9 +573,17 @@ public void resetPartition(String clusterName, String instanceName, String resou
// check the instance is alive
LiveInstance liveInstance = accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + " on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has ever exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName, instanceName);
+ if (!_zkClient.exists(instancePath)) {
+ throw new HelixException(
+ "Can't reset state for " + resourceName + "/" + partitionNames + " on " + instanceName
+ + ", because " + instanceName + " never existed in cluster " + clusterName);
+ } else {
+ throw new HelixException(
+ "Can't reset state for " + resourceName + "/" + partitionNames + " on " + instanceName
+ + ", because " + instanceName + " is not alive in cluster " + clusterName + " anymore");
Review comment:
"not live"
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -573,9 +573,17 @@ public void resetPartition(String clusterName, String instanceName, String resou
// check the instance is alive
LiveInstance liveInstance = accessor.getProperty(keyBuilder.liveInstance(instanceName));
if (liveInstance == null) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + " on " + instanceName
- + ", because " + instanceName + " is not alive");
+ // check if the instance has ever exist in the cluster
+ String instancePath = PropertyPathBuilder.instance(clusterName, instanceName);
+ if (!_zkClient.exists(instancePath)) {
+ throw new HelixException(
+ "Can't reset state for " + resourceName + "/" + partitionNames + " on " + instanceName
+ + ", because " + instanceName + " never existed in cluster " + clusterName);
+ } else {
+ throw new HelixException(
+ "Can't reset state for " + resourceName + "/" + partitionNames + " on " + instanceName
+ + ", because " + instanceName + " is not alive in cluster " + clusterName + " anymore");
Review comment:
Can you just create the error message in the if-else block and throw only once to avoid code duplication?
##########
File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java
##########
@@ -560,6 +560,93 @@ public void testLegacyEnableDisablePartition() {
2);
}
+ @Test
+ public void testResetPartition() {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+ String instanceName = "TestInstance";
+ String testResource = "TestResource";
+ System.out.println("START " + clusterName + " at " + new Date(System.currentTimeMillis()));
+ HelixAdmin admin = new ZKHelixAdmin(_gZkClient);
+ admin.addCluster(clusterName, true);
+ admin.addInstance(clusterName, new InstanceConfig(instanceName));
+ admin.enableInstance(clusterName, instanceName, true);
+
+ InstanceConfig instanceConfig = admin.getInstanceConfig(clusterName, instanceName);
+
+ // Test the sanity check for resetPartition.
+ // resetPartition is expected to throw an exception when provided with a non-exist instance.
Review comment:
"nonexistent"
##########
File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java
##########
@@ -560,6 +560,93 @@ public void testLegacyEnableDisablePartition() {
2);
}
+ @Test
+ public void testResetPartition() {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+ String instanceName = "TestInstance";
+ String testResource = "TestResource";
+ System.out.println("START " + clusterName + " at " + new Date(System.currentTimeMillis()));
+ HelixAdmin admin = new ZKHelixAdmin(_gZkClient);
+ admin.addCluster(clusterName, true);
+ admin.addInstance(clusterName, new InstanceConfig(instanceName));
+ admin.enableInstance(clusterName, instanceName, true);
+
+ InstanceConfig instanceConfig = admin.getInstanceConfig(clusterName, instanceName);
+
+ // Test the sanity check for resetPartition.
+ // resetPartition is expected to throw an exception when provided with a non-exist instance.
+ try {
+ admin.resetPartition(clusterName, "WrongTestInstance", testResource,
+ Arrays.asList("1", "2"));
+ Assert.fail("Should throw HelixException");
+ } catch (HelixException expected) {
+ // This exception is expected because the instance name is made up.
+ Assert.assertEquals(expected.getMessage(),
+ "Can't reset state for " + testResource + "/[1, 2] on WrongTestInstance," +
+ " because WrongTestInstance never existed in cluster " + clusterName);
+ }
+
+ // resetPartition is expected to throw an exception when provided with a non-alive instance.
Review comment:
non-live
##########
File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java
##########
@@ -560,6 +560,93 @@ public void testLegacyEnableDisablePartition() {
2);
}
+ @Test
+ public void testResetPartition() {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+ String instanceName = "TestInstance";
+ String testResource = "TestResource";
+ System.out.println("START " + clusterName + " at " + new Date(System.currentTimeMillis()));
+ HelixAdmin admin = new ZKHelixAdmin(_gZkClient);
+ admin.addCluster(clusterName, true);
+ admin.addInstance(clusterName, new InstanceConfig(instanceName));
+ admin.enableInstance(clusterName, instanceName, true);
+
+ InstanceConfig instanceConfig = admin.getInstanceConfig(clusterName, instanceName);
+
+ // Test the sanity check for resetPartition.
+ // resetPartition is expected to throw an exception when provided with a non-exist instance.
+ try {
+ admin.resetPartition(clusterName, "WrongTestInstance", testResource,
+ Arrays.asList("1", "2"));
+ Assert.fail("Should throw HelixException");
+ } catch (HelixException expected) {
+ // This exception is expected because the instance name is made up.
+ Assert.assertEquals(expected.getMessage(),
+ "Can't reset state for " + testResource + "/[1, 2] on WrongTestInstance," +
+ " because WrongTestInstance never existed in cluster " + clusterName);
+ }
+
+ // resetPartition is expected to throw an exception when provided with a non-alive instance.
+ try {
+ admin.resetPartition(clusterName, instanceName, testResource,
+ Arrays.asList("1", "2"));
+ Assert.fail("Should throw HelixException");
+ } catch (HelixException expected) {
+ // This exception is expected because the instance is not alive.
+ Assert.assertEquals(expected.getMessage(),
+ "Can't reset state for " + testResource + "/[1, 2] on " + instanceName
+ + ", because " + instanceName + " is not alive in cluster " + clusterName + " anymore");
Review comment:
not live
----------------------------------------------------------------
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