Add protective check for ZKHelixAdmin.dropInstance() Dropping an instance when it is still alive could result in inconsistent state and should not be allowed. Add protective check and throw exception when this happens. Add test for this check. Start a process to make the instance alive, try dropping the instance(fail as expected), then stop the process and drop the instance as usual.
Project: http://git-wip-us.apache.org/repos/asf/helix/repo Commit: http://git-wip-us.apache.org/repos/asf/helix/commit/79ebc046 Tree: http://git-wip-us.apache.org/repos/asf/helix/tree/79ebc046 Diff: http://git-wip-us.apache.org/repos/asf/helix/diff/79ebc046 Branch: refs/heads/master Commit: 79ebc0469da73a7a1b2d8c73a42efc4001d06088 Parents: d51f353 Author: Weihan Kong <[email protected]> Authored: Mon Sep 25 17:59:00 2017 -0700 Committer: Junkai Xue <[email protected]> Committed: Mon Sep 25 17:59:00 2017 -0700 ---------------------------------------------------------------------- .../apache/helix/manager/zk/ZKHelixAdmin.java | 19 +-- .../helix/manager/zk/TestZkHelixAdmin.java | 122 +++++++++---------- 2 files changed, 68 insertions(+), 73 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/helix/blob/79ebc046/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java ---------------------------------------------------------------------- diff --git a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java index 351c10e..19a4193 100644 --- a/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java +++ b/helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java @@ -121,24 +121,27 @@ public class ZKHelixAdmin implements HelixAdmin { @Override public void dropInstance(String clusterName, InstanceConfig instanceConfig) { - String instanceConfigsPath = - PropertyPathBuilder.getPath(PropertyType.CONFIGS, clusterName, - ConfigScopeProperty.PARTICIPANT.toString()); - String nodeId = instanceConfig.getId(); - String instanceConfigPath = instanceConfigsPath + "/" + nodeId; - String instancePath = PropertyPathBuilder.instance(clusterName, nodeId); + String instanceName = instanceConfig.getInstanceName(); + String instanceConfigPath = PropertyPathBuilder.instanceConfig(clusterName, instanceName); if (!_zkClient.exists(instanceConfigPath)) { - throw new HelixException("Node " + nodeId + " does not exist in config for cluster " + throw new HelixException("Node " + instanceName + " does not exist in config for cluster " + clusterName); } + String instancePath = PropertyPathBuilder.instance(clusterName, instanceName); if (!_zkClient.exists(instancePath)) { - throw new HelixException("Node " + nodeId + " does not exist in instances for cluster " + throw new HelixException("Node " + instanceName + " does not exist in instances for cluster " + clusterName); } + String liveInstancePath = PropertyPathBuilder.liveInstance(clusterName, instanceName); + if (_zkClient.exists(liveInstancePath)) { + throw new HelixException("Node " + instanceName + " is still alive for cluster " + clusterName + ", can't drop."); + } + // delete config path + String instanceConfigsPath = PropertyPathBuilder.instanceConfig(clusterName); ZKUtil.dropChildren(_zkClient, instanceConfigsPath, instanceConfig.getRecord()); // delete instance path http://git-wip-us.apache.org/repos/asf/helix/blob/79ebc046/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java ---------------------------------------------------------------------- diff --git a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java index a431171..236a5b2 100644 --- a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java +++ b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java @@ -29,12 +29,16 @@ import org.apache.helix.BaseDataAccessor; import org.apache.helix.HelixAdmin; import org.apache.helix.HelixDataAccessor; import org.apache.helix.HelixException; +import org.apache.helix.HelixManager; +import org.apache.helix.HelixManagerFactory; +import org.apache.helix.InstanceType; import org.apache.helix.PropertyKey; import org.apache.helix.PropertyPathBuilder; import org.apache.helix.PropertyType; import org.apache.helix.TestHelper; import org.apache.helix.ZNRecord; import org.apache.helix.ZkUnitTestBase; +import org.apache.helix.examples.MasterSlaveStateModelFactory; import org.apache.helix.model.ClusterConstraints; import org.apache.helix.model.ClusterConstraints.ConstraintAttribute; import org.apache.helix.model.ClusterConstraints.ConstraintType; @@ -47,6 +51,7 @@ import org.apache.helix.model.InstanceConfig; import org.apache.helix.model.StateModelDefinition; import org.apache.helix.model.builder.ConstraintItemBuilder; import org.apache.helix.model.builder.HelixConfigScopeBuilder; +import org.apache.helix.participant.StateMachineEngine; import org.apache.helix.tools.StateModelConfigGenerator; import org.apache.zookeeper.data.Stat; import org.testng.Assert; @@ -54,8 +59,8 @@ import org.testng.AssertJUnit; import org.testng.annotations.Test; public class TestZkHelixAdmin extends ZkUnitTestBase { - @Test() public void testZkHelixAdmin() { + //TODO refactor this test into small test cases and use @before annotations System.out.println("START testZkHelixAdmin at " + new Date(System.currentTimeMillis())); final String clusterName = getShortClassName(); @@ -85,19 +90,12 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { // OK } - String hostname = "host1"; - String port = "9999"; - String instanceName = hostname + "_" + port; - InstanceConfig config = new InstanceConfig(instanceName); - config.setHostName(hostname); - config.setPort(port); - List<String> dummyList = new ArrayList<String>(); - dummyList.add("foo"); - dummyList.add("bar"); - config.getRecord().setListField("dummy", dummyList); + InstanceConfig config = new InstanceConfig("host1_9999"); + config.setHostName("host1"); + config.setPort("9999"); tool.addInstance(clusterName, config); - tool.enableInstance(clusterName, instanceName, true); - String path = PropertyPathBuilder.getPath(PropertyType.INSTANCES, clusterName, instanceName); + tool.enableInstance(clusterName, "host1_9999", true); + String path = PropertyPathBuilder.instance(clusterName, "host1_9999"); AssertJUnit.assertTrue(_gZkClient.exists(path)); try { @@ -106,43 +104,32 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { } catch (HelixException e) { // OK } - config = tool.getInstanceConfig(clusterName, instanceName); - AssertJUnit.assertEquals(config.getId(), instanceName); + config = tool.getInstanceConfig(clusterName, "host1_9999"); + AssertJUnit.assertEquals(config.getId(), "host1_9999"); - // test setInstanceConfig() - config = tool.getInstanceConfig(clusterName, instanceName); - config.setHostName("host2"); + // test: should not drop instance when it is still alive + HelixManager manager = initializeHelixManager(clusterName, config.getInstanceName(), ZK_ADDR, "id1"); try { - // different host - tool.setInstanceConfig(clusterName, instanceName, config); - Assert.fail("should fail if hostname is different from the current one"); - } catch (HelixException e) { - // OK + manager.connect(); + } catch (Exception e) { + Assert.fail("HelixManager failed connecting"); } - config = tool.getInstanceConfig(clusterName, instanceName); - config.setPort("7777"); try { - // different port - tool.setInstanceConfig(clusterName, instanceName, config); - Assert.fail("should fail if port is different from the current one"); + tool.dropInstance(clusterName, config); + Assert.fail("should fail if an instance is still alive"); } catch (HelixException e) { // OK } - dummyList.remove("bar"); - dummyList.add("baz"); - config = tool.getInstanceConfig(clusterName, instanceName); - config.getRecord().setListField("dummy", dummyList); - AssertJUnit.assertTrue(tool.setInstanceConfig(clusterName, "host1_9999", config)); - config = tool.getInstanceConfig(clusterName, "host1_9999"); - dummyList = config.getRecord().getListField("dummy"); - AssertJUnit.assertTrue(dummyList.contains("foo")); - AssertJUnit.assertTrue(dummyList.contains("baz")); - AssertJUnit.assertFalse(dummyList.contains("bar")); - AssertJUnit.assertEquals(2, dummyList.size()); + try { + manager.disconnect(); + } catch (Exception e) { + Assert.fail("HelixManager failed disconnecting"); + } + + tool.dropInstance(clusterName, config); // correctly drop the instance - tool.dropInstance(clusterName, config); try { tool.getInstanceConfig(clusterName, "host1_9999"); Assert.fail("should fail if get a non-existent instance"); @@ -164,7 +151,7 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { ZNRecord stateModelRecord = new ZNRecord("id1"); try { tool.addStateModelDef(clusterName, "id1", new StateModelDefinition(stateModelRecord)); - path = PropertyPathBuilder.getPath(PropertyType.STATEMODELDEFS, clusterName, "id1"); + path = PropertyPathBuilder.stateModelDef(clusterName, "id1"); AssertJUnit.assertTrue(_gZkClient.exists(path)); Assert.fail("should fail"); } catch (HelixException e) { @@ -233,6 +220,18 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { System.out.println("END testZkHelixAdmin at " + new Date(System.currentTimeMillis())); } + private HelixManager initializeHelixManager(String clusterName, String instanceName, String zkAddress, + String stateModelName) { + HelixManager manager = + HelixManagerFactory.getZKHelixManager(clusterName, instanceName, InstanceType.PARTICIPANT, zkAddress); + + MasterSlaveStateModelFactory stateModelFactory = new MasterSlaveStateModelFactory(instanceName); + + StateMachineEngine stateMach = manager.getStateMachineEngine(); + stateMach.registerStateModelFactory(stateModelName, stateModelFactory); + return manager; + } + // drop resource should drop corresponding resource-level config also @Test public void testDropResource() { @@ -246,13 +245,13 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { tool.addCluster(clusterName, true); Assert.assertTrue(ZKUtil.isClusterSetup(clusterName, _gZkClient), "Cluster should be setup"); - tool.addStateModelDef(clusterName, "MasterSlave", new StateModelDefinition( - StateModelConfigGenerator.generateConfigForMasterSlave())); + tool.addStateModelDef(clusterName, "MasterSlave", new StateModelDefinition(StateModelConfigGenerator.generateConfigForMasterSlave())); tool.addResource(clusterName, "test-db", 4, "MasterSlave"); Map<String, String> resourceConfig = new HashMap<String, String>(); resourceConfig.put("key1", "value1"); - tool.setConfig(new HelixConfigScopeBuilder(ConfigScopeProperty.RESOURCE) - .forCluster(clusterName).forResource("test-db").build(), resourceConfig); + tool.setConfig(new HelixConfigScopeBuilder(ConfigScopeProperty.RESOURCE).forCluster(clusterName) + .forResource("test-db") + .build(), resourceConfig); PropertyKey.Builder keyBuilder = new PropertyKey.Builder(clusterName); Assert.assertTrue(_gZkClient.exists(keyBuilder.idealStates("test-db").getPath()), @@ -283,8 +282,7 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { Assert.assertTrue(ZKUtil.isClusterSetup(clusterName, _gZkClient), "Cluster should be setup"); // test admin.getMessageConstraints() - ClusterConstraints constraints = - tool.getConstraints(clusterName, ConstraintType.MESSAGE_CONSTRAINT); + ClusterConstraints constraints = tool.getConstraints(clusterName, ConstraintType.MESSAGE_CONSTRAINT); Assert.assertNull(constraints, "message-constraint should NOT exist for cluster: " + className); // remove non-exist constraint @@ -299,14 +297,11 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { ConstraintItemBuilder builder = new ConstraintItemBuilder(); builder.addConstraintAttribute(ConstraintAttribute.RESOURCE.toString(), "MyDB") .addConstraintAttribute(ConstraintAttribute.CONSTRAINT_VALUE.toString(), "1"); - tool.setConstraint(clusterName, ConstraintType.MESSAGE_CONSTRAINT, "constraint1", - builder.build()); + tool.setConstraint(clusterName, ConstraintType.MESSAGE_CONSTRAINT, "constraint1", builder.build()); - HelixDataAccessor accessor = - new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(_gZkClient)); + HelixDataAccessor accessor = new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(_gZkClient)); PropertyKey.Builder keyBuilder = new PropertyKey.Builder(clusterName); - constraints = - accessor.getProperty(keyBuilder.constraint(ConstraintType.MESSAGE_CONSTRAINT.toString())); + constraints = accessor.getProperty(keyBuilder.constraint(ConstraintType.MESSAGE_CONSTRAINT.toString())); Assert.assertNotNull(constraints, "message-constraint should exist"); ConstraintItem item = constraints.getConstraintItem("constraint1"); Assert.assertNotNull(item, "message-constraint for constraint1 should exist"); @@ -323,8 +318,7 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { // remove a exist message-constraint tool.removeConstraint(clusterName, ConstraintType.MESSAGE_CONSTRAINT, "constraint1"); - constraints = - accessor.getProperty(keyBuilder.constraint(ConstraintType.MESSAGE_CONSTRAINT.toString())); + constraints = accessor.getProperty(keyBuilder.constraint(ConstraintType.MESSAGE_CONSTRAINT.toString())); Assert.assertNotNull(constraints, "message-constraint should exist"); item = constraints.getConstraintItem("constraint1"); Assert.assertNull(item, "message-constraint for constraint1 should NOT exist"); @@ -342,8 +336,7 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { admin.addCluster(clusterName, true); Assert.assertTrue(ZKUtil.isClusterSetup(clusterName, _gZkClient), "Cluster should be setup"); String resourceName = "TestDB"; - admin.addStateModelDef(clusterName, "MasterSlave", new StateModelDefinition( - StateModelConfigGenerator.generateConfigForMasterSlave())); + admin.addStateModelDef(clusterName, "MasterSlave", new StateModelDefinition(StateModelConfigGenerator.generateConfigForMasterSlave())); admin.addResource(clusterName, resourceName, 4, "MasterSlave"); admin.enableResource(clusterName, resourceName, false); BaseDataAccessor<ZNRecord> baseAccessor = new ZkBaseDataAccessor<ZNRecord>(_gZkClient); @@ -408,6 +401,7 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { AssertJUnit.assertEquals(allResources.size(), 4); AssertJUnit.assertEquals(resourcesWithTag.size(), 2); } + @Test public void testEnableDisablePartitions() { String className = TestHelper.getTestClassName(); @@ -441,20 +435,18 @@ public class TestZkHelixAdmin extends ZkUnitTestBase { String instanceName = "TestInstanceLegacy"; String testResourcePrefix = "TestResourceLegacy"; ZNRecord record = new ZNRecord(instanceName); - List<String> disabledPartitions = - new ArrayList<String>(Arrays.asList(new String[] { "1", "2", "3" })); - record.setListField(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_PARTITION.name(), - disabledPartitions); + List<String> disabledPartitions = new ArrayList<String>(Arrays.asList(new String[]{"1", "2", "3"})); + record.setListField(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_PARTITION.name(), disabledPartitions); InstanceConfig instanceConfig = new InstanceConfig(record); instanceConfig.setInstanceEnabledForPartition(testResourcePrefix, "2", false); Assert.assertEquals(instanceConfig.getDisabledPartitions(testResourcePrefix).size(), 3); Assert.assertEquals(instanceConfig.getRecord() - .getListField(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_PARTITION.name()).size(), - 3); + .getListField(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_PARTITION.name()) + .size(), 3); instanceConfig.setInstanceEnabledForPartition(testResourcePrefix, "2", true); Assert.assertEquals(instanceConfig.getDisabledPartitions(testResourcePrefix).size(), 2); Assert.assertEquals(instanceConfig.getRecord() - .getListField(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_PARTITION.name()).size(), - 2); + .getListField(InstanceConfig.InstanceConfigProperty.HELIX_DISABLED_PARTITION.name()) + .size(), 2); } }
