pkuwm commented on a change in pull request #1129:
URL: https://github.com/apache/helix/pull/1129#discussion_r460488234
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -20,14 +20,17 @@
*/
import java.io.IOException;
+import java.security.InvalidParameterException;
Review comment:
Seems this import is unused?
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -43,11 +43,11 @@
*/
public class Topology {
private static Logger logger = LoggerFactory.getLogger(Topology.class);
+
public enum Types {
- ROOT,
- ZONE,
- INSTANCE
+ ROOT, ZONE, INSTANCE
Review comment:
IntelliJ formats the style. But maybe unnecessary. It is recommended to
only format your changes. :)
##########
File path: helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java
##########
@@ -637,4 +638,19 @@ public static InstanceConfig toInstanceConfig(String
instanceId) {
}
return config;
}
+
+ /**
+ * Validate if the topology related settings (Domain or ZoneId) in the given
instanceConfig
+ * are valid and align with current clusterConfig.
+ * This function should be called when instance added to cluster or caller
updates instanceConfig.
+ *
+ * @throws IllegalArgumentException
+ */
+ public boolean validateTopologySettingInInstanceConfig(ClusterConfig
clusterConfig,
+ String instanceName) throws IllegalArgumentException{
Review comment:
Usually runtime exception is not recommended to put in method signature.
Having it in java doc is good enough.
##########
File path:
helix-core/src/test/java/org/apache/helix/integration/manager/TestParticipantManager.java
##########
@@ -114,6 +118,50 @@ public void simpleIntegrationTest() throws Exception {
Assert.assertNull(accessor.getProperty(keyBuilder.controllerLeader()));
}
+ @Test
+ public void simpleIntegrationTestNeg() throws Exception {
+
+ TestHelper.setupCluster(clusterName, ZK_ADDR, 12918, // participant port
+ "localhost", // participant name prefix
+ "TestDB", // resource name prefix
+ 1, // resources
+ 4, // partitions per resource
+ 1, // number of nodes
+ 1, // replicas
+ "MasterSlave", true); // do rebalance
+
+ ConfigAccessor configAccessor = new ConfigAccessor(_gZkClient);
+ ClusterConfig clusterConfig = configAccessor.getClusterConfig(clusterName);
+ clusterConfig.getRecord()
+
.setListField(ClusterConfig.ClusterConfigProperty.INSTANCE_CAPACITY_KEYS.name(),
+ new ArrayList<>());
+ clusterConfig.setTopologyAwareEnabled(true);
+ clusterConfig.setTopology("/Rack/Sub-Rack/Host/Instance");
+ clusterConfig.setFaultZoneType("Host");
+ configAccessor.setClusterConfig(clusterName, clusterConfig);
+
+
+ String instanceName = "localhost_12918";
+ HelixManager participant =
+ new ZKHelixManager(clusterName, instanceName ,
InstanceType.PARTICIPANT, ZK_ADDR);
+
participant.getStateMachineEngine().registerStateModelFactory("MasterSlave",
+ new MockMSModelFactory());
+ // We are expecting an IllegalArgumentException since the domain is not
set.
+ try {
+ participant.connect();
Review comment:
Assert fail here to protect the test. Otherwise if expecting exception
is not throw, the test will still pass. It should 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]
