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 20:46:42 UTC
[GitHub] [helix] qqu0127 opened a new pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
qqu0127 opened a new pull request #1945:
URL: https://github.com/apache/helix/pull/1945
A refactoring for virtual group project.
We found a need to reference class ClusterTopologyConfig outside of the scope of controller or rebalancer. Thus we refactor it out to a separate class and do some cleanup. This should be backward compatible.
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
We move ClusterTopologyConfig from nested to a standalone class in helix/model and to be used by virtual topology group logic. Some code refactor and cleanup are included.
### Tests
- [ ] The following tests are written for this issue:
Add new UT class TestClusterTopologyConfig
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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
[GitHub] [helix] qqu0127 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793083225
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1033,6 +1033,14 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ /**
+ * Create an instance of {@link ClusterTopologyConfig} based on cluster config.
+ * @return an instance of {@link ClusterTopologyConfig}.
+ */
+ public ClusterTopologyConfig getClusterTopologyConfig() {
+ return ClusterTopologyConfig.fromClusterConfig(this);
Review comment:
Good question, indeed it's not the most beautiful way... I choose this approach to avoid over expose this class, right now `ClusterTopologyConfig.fromClusterConfig` is package private.
In my view, this class is like a view of `ClusterConfig`, a container associated with ClusterConfig, and how we construct this is impl details and shouldn't be exposed outside package. I didn't write the logic directly in ClusterConfig because this class is already too long.
This is open to discussion, let me know how you feel about it. Thanks!
--
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
[GitHub] [helix] qqu0127 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793078553
##########
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:
This is what I read https://github.com/apache/helix/pull/1945/files#diff-3bfc7aaa9b4578cd05434a4b4564dd61aa561f340ba7c08d1395d3f50a881fc2L238-L243, so I just keep the behavior.
Since I'm not really sure on the impact of changing this default value, I prefer keeping it the same.
@xyuanlu any input? I saw you introduced [this](https://github.com/apache/helix/commit/0869e729530d83ba55228b2d44a79ae44175c081).
--
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
[GitHub] [helix] qqu0127 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793072270
##########
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:
Good catch, let me revert to IllegalArgumentException
--
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
[GitHub] [helix] qqu0127 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793070890
##########
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:
Thanks, will update
--
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
[GitHub] [helix] xyuanlu commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r794052496
##########
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:
The default fault zone type and end node type is INSTANCE when topologyAware is not enabled. When it is enabled, the default fault zone type is ZONE and end node type is INSTANCE.
This behavior is added in 2017 by change https://github.com/apache/helix/commit/cb024650c25c6ddee7348d37fb67fa1a2e93bff1#
IMHO, the current behavior make sense. When no topology enabled, the structure should be flat, meaning zone and end node should be the same.
--
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
[GitHub] [helix] qqu0127 commented on pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on pull request #1945:
URL: https://github.com/apache/helix/pull/1945#issuecomment-1024494831
Ready to merge, approved by @NealSun96
Final commit message:
Fix apache#1946 Refactor and move ClusterTopologyConfig
--
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793073771
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1033,6 +1033,14 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ /**
+ * Create an instance of {@link ClusterTopologyConfig} based on cluster config.
+ * @return an instance of {@link ClusterTopologyConfig}.
+ */
+ public ClusterTopologyConfig getClusterTopologyConfig() {
+ return ClusterTopologyConfig.fromClusterConfig(this);
Review comment:
Seems a bit odd to me... Is there a reason why ClusterConfig has a method to construct a ClusterTopologyConfig out of itself? If ClusterTopologyConfig needs to be constructed, the caller should just directly call `ClusterTopologyConfig.fromClusterConfig(clusterConfig)`, instead of `clusterConfig.getClusterTopologyConfig()`; this method doesn't seem that necessary to me.
--
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
[GitHub] [helix] qqu0127 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793956443
##########
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:
TestTopology.testCreateClusterTopologyWithDefaultTopology PASS with the patch.
--
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793165688
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -54,14 +55,7 @@
private final List<String> _liveInstances;
private final Map<String, InstanceConfig> _instanceConfigMap;
private final ClusterConfig _clusterConfig;
Review comment:
Can put all the related info to ClusterTopologyConfig and remove the reference of ClusterConfig 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
[GitHub] [helix] NealSun96 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793949652
##########
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:
@qqu0127 I agree that @xyuanlu should take a look. Could anyone of you check if `testCreateClusterTopologyWithDefaultTopology` is correctly testing this case, by the way? On the first glance that should have caught this case? Thanks.
--
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
[GitHub] [helix] NealSun96 commented on pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on pull request #1945:
URL: https://github.com/apache/helix/pull/1945#issuecomment-1023577180
@qqu0127 Looks good to me except the issue @zhangmeng916 raised. Please also check the boxes in the PR description before final checkin (btw, I think the first paragraph you have should be merged into the Description section).
--
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793156925
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1033,6 +1033,14 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ /**
+ * Create an instance of {@link ClusterTopologyConfig} based on cluster config.
+ * @return an instance of {@link ClusterTopologyConfig}.
+ */
+ public ClusterTopologyConfig getClusterTopologyConfig() {
+ return ClusterTopologyConfig.fromClusterConfig(this);
Review comment:
I believe ClusterTopologyConfig is moved out of the class to make it independent. It makes more sense to me that ClusterTopologyConfig class is called to create itself.
--
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
[GitHub] [helix] qqu0127 edited a comment on pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 edited a comment on pull request #1945:
URL: https://github.com/apache/helix/pull/1945#issuecomment-1024494831
Ready to merge, approved by @NealSun96
Final commit message:
Fix apache#1946 Refactor and move ClusterTopologyConfig
Move ClusterTopologyConfig from nested to a standalone class in helix/model and to be used by virtual topology group logic.
--
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
[GitHub] [helix] NealSun96 merged pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
NealSun96 merged pull request #1945:
URL: https://github.com/apache/helix/pull/1945
--
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [helix] qqu0127 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793206288
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/topology/Topology.java
##########
@@ -54,14 +55,7 @@
private final List<String> _liveInstances;
private final Map<String, InstanceConfig> _instanceConfigMap;
private final ClusterConfig _clusterConfig;
Review comment:
Good point, updated. thx
--
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
[GitHub] [helix] qqu0127 commented on a change in pull request #1945: Refactor and move ClusterTopologyConfig from nested class to a separate one.
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #1945:
URL: https://github.com/apache/helix/pull/1945#discussion_r793206422
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1033,6 +1033,14 @@ public void setAbnormalStateResolverMap(Map<String, String> resolverMap) {
return idealStateRuleMap;
}
+ /**
+ * Create an instance of {@link ClusterTopologyConfig} based on cluster config.
+ * @return an instance of {@link ClusterTopologyConfig}.
+ */
+ public ClusterTopologyConfig getClusterTopologyConfig() {
+ return ClusterTopologyConfig.fromClusterConfig(this);
Review comment:
Fair enough, updated. Thanks.
--
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