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 2022/01/26 21:30:41 UTC
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
zhangmeng916 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793052469
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterTopologyConfig.java
##########
@@ -0,0 +1,92 @@
+package org.apache.helix.model;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import com.google.common.base.Preconditions;
+import java.util.LinkedHashMap;
+import org.apache.helix.HelixException;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+
+
+public class ClusterTopologyConfig {
+ private static final String DEFAULT_DOMAIN_PREFIX = "Helix_default_";
+ private static final String TOPOLOGY_SPLITTER = "/";
+
+ private final String _endNodeType;
+ private final String _faultZoneType;
+ private final LinkedHashMap<String, String> _topologyKeyDefaultValue;
+
+ private ClusterTopologyConfig(String endNodeType, String faultZoneType,
+ LinkedHashMap<String, String> topologyKeyDefaultValue) {
+ _endNodeType = endNodeType;
+ _faultZoneType = faultZoneType;
+ _topologyKeyDefaultValue = topologyKeyDefaultValue;
+ }
+
+ /**
+ * Populate faultZone, endNodetype and and a LinkedHashMap containing pathKeys default values for
+ * clusterConfig.Topology. The LinkedHashMap will be empty if clusterConfig.Topology is unset.
+ *
+ * @return an instance of {@link ClusterTopologyConfig}
+ */
+ static ClusterTopologyConfig fromClusterConfig(ClusterConfig clusterConfig) {
Review comment:
The naming of the function does not comply with the standard. Suggest to rename as "createFromClusterConfig" or similar.
##########
File path: helix-core/src/test/java/org/apache/helix/model/TestClusterTopologyConfig.java
##########
@@ -0,0 +1,84 @@
+package org.apache.helix.model;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Iterator;
+import org.apache.helix.HelixException;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestClusterTopologyConfig {
+
+ @Test
+ public void testClusterNonTopologyAware() {
+ ClusterConfig testConfig = new ClusterConfig("testId");
+ testConfig.setTopologyAwareEnabled(false);
+ ClusterTopologyConfig clusterTopologyConfig = ClusterTopologyConfig.fromClusterConfig(testConfig);
+ Assert.assertEquals(clusterTopologyConfig.getEndNodeType(), Topology.Types.INSTANCE.name());
+ Assert.assertEquals(clusterTopologyConfig.getFaultZoneType(), Topology.Types.INSTANCE.name());
Review comment:
Is this correct? Why the fault zone type is INSTANCE name? shall we change the default value to be meaningful?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterTopologyConfig.java
##########
@@ -0,0 +1,92 @@
+package org.apache.helix.model;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import com.google.common.base.Preconditions;
+import java.util.LinkedHashMap;
+import org.apache.helix.HelixException;
+import org.apache.helix.controller.rebalancer.topology.Topology;
+
+
+public class ClusterTopologyConfig {
+ private static final String DEFAULT_DOMAIN_PREFIX = "Helix_default_";
+ private static final String TOPOLOGY_SPLITTER = "/";
+
+ private final String _endNodeType;
+ private final String _faultZoneType;
+ private final LinkedHashMap<String, String> _topologyKeyDefaultValue;
+
+ private ClusterTopologyConfig(String endNodeType, String faultZoneType,
+ LinkedHashMap<String, String> topologyKeyDefaultValue) {
+ _endNodeType = endNodeType;
+ _faultZoneType = faultZoneType;
+ _topologyKeyDefaultValue = topologyKeyDefaultValue;
+ }
+
+ /**
+ * Populate faultZone, endNodetype and and a LinkedHashMap containing pathKeys default values for
+ * clusterConfig.Topology. The LinkedHashMap will be empty if clusterConfig.Topology is unset.
+ *
+ * @return an instance of {@link ClusterTopologyConfig}
+ */
+ static ClusterTopologyConfig fromClusterConfig(ClusterConfig clusterConfig) {
+ if (!clusterConfig.isTopologyAwareEnabled()) {
+ return new ClusterTopologyConfig(
+ Topology.Types.INSTANCE.name(), Topology.Types.INSTANCE.name(), new LinkedHashMap<>());
+ }
+ // Assign default cluster topology definition, i,e. /root/zone/instance
+ String endNodeType = Topology.Types.INSTANCE.name();
+ String faultZoneType = Topology.Types.ZONE.name();
+ LinkedHashMap<String, String> topologyKeyDefaultValue = new LinkedHashMap<>();
+
+ String topologyDef = clusterConfig.getTopology();
+ if (topologyDef != null) {
+ for (String topologyKey : topologyDef.trim().split(TOPOLOGY_SPLITTER)) {
+ if (!topologyKey.isEmpty()) {
+ topologyKeyDefaultValue.put(topologyKey, DEFAULT_DOMAIN_PREFIX + topologyKey);
+ endNodeType = topologyKey;
+ }
+ }
+ Preconditions.checkState(!topologyKeyDefaultValue.isEmpty(),
Review comment:
This check will throws an illegal statement exception, while the original one throws illegal argument exception. Is this what you planned to change? I feel illegal argument is more proper here.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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