pkuwm commented on a change in pull request #1007:
URL: https://github.com/apache/helix/pull/1007#discussion_r424741712
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -588,20 +596,16 @@ public void resetPartition(String clusterName, String
instanceName, String resou
// check partition exists in resource group
Set<String> resetPartitionNames = new HashSet<String>(partitionNames);
+ Set<String> partitions;
if (idealState.getRebalanceMode() == RebalanceMode.CUSTOMIZED) {
- Set<String> partitions = new
HashSet<String>(idealState.getRecord().getMapFields().keySet());
- if (!partitions.containsAll(resetPartitionNames)) {
- throw new HelixException(
- "Can't reset state for " + resourceName + "/" + partitionNames + "
on " + instanceName
- + ", because not all " + partitionNames + " exist");
- }
+ partitions = new
HashSet<String>(idealState.getRecord().getMapFields().keySet());
Review comment:
Suggest diamond `new HashSet<>()`
And it seems the partitions are only used for next step `containsAll()`.
Would it be fine just use the reference to keySet() `partitions = Address
unclean helix controller leadership switch`, instead of making a copy?
##########
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");
+ }
+
+ HelixManager manager = initializeHelixManager(clusterName,
instanceConfig.getInstanceName());
+ try {
+ manager.connect();
+ } catch (Exception e) {
Review comment:
No need to catch this exception. If an exception is thrown from
connect(), the test will throw this exception and fail.
----------------------------------------------------------------
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]
