jiajunwang commented on a change in pull request #1007:
URL: https://github.com/apache/helix/pull/1007#discussion_r426823140
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -560,6 +560,51 @@ private void processMaintenanceMode(String clusterName,
final boolean enabled,
}
}
+ private enum ResetPartitionFailureReason {
+ INSTANCE_NOTALIVE,
+ INSTANCE_NONEXISTENT,
+ RESOURCE_NONEXISTENT,
+ PARTITION_NONEXISTENT,
+ PARTITION_NOT_ERROR,
+ STATE_MODEL_NONEXISTENT,
+ PENDING_MSG
+ }
+
+ private String resetPartitionLogHelper(String clusterName, String
instanceName,
+ String resourceName, List<String> partitionNames, String strArg,
+ ResetPartitionFailureReason reason) {
+ String errMessage = String
+ .format("Can't reset state for %s.%s on %s, because ", resourceName,
partitionNames,
+ instanceName);
+
+ switch (reason) {
+ case INSTANCE_NOTALIVE:
+ errMessage +=
+ String.format("%s has never existed in cluster %s", instanceName,
clusterName);
Review comment:
nit, this is about style.
In Java, you can put fields into enum items. This format string can be a
field of the ResetPartitionFailureReason enum item. Then you don't need the
switch here.
##########
File path:
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java
##########
@@ -560,6 +560,82 @@ public void testLegacyEnableDisablePartition() {
2);
}
+ @Test
+ public void testResetPartition() throws Exception {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+ String instanceName = "TestInstance";
+ String testResource = "TestResource";
+ String wrongTestInstance = "WrongTestInstance";
+ 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
nonexistent instance.
+ try {
+ admin.resetPartition(clusterName, wrongTestInstance, testResource,
Arrays.asList("1", "2"));
Review comment:
This test case works but not very clean. Since the testResource has not
been created, the resetPartition may fail due to other reasons. So the test
logic here might be impacted if we change that logic. For example, we check the
resource existence first in the future.
To avoid that impact, let's have the resource created and then test reset.
BTW, in that case, you will need to use the verifier to wait until resource
assignment has been done and state converged.
----------------------------------------------------------------
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]
