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/04/04 23:19:03 UTC
[GitHub] [helix] xyuanlu opened a new pull request, #2015: Refactor common string processing util
xyuanlu opened a new pull request, #2015:
URL: https://github.com/apache/helix/pull/2015
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
#2000
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This change refactors common code to a util to be used in following change that does batch instances disable/enable.
### Tests
- [X] The following tests are written for this issue:
NA
Code refactor. Util functuons are tested in caller.
- 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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843062536
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,64 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class HelixConfigStringProcessUtil {
Review Comment:
There are commonly-followed Java idiom for creating util classes (`final` class, private constructor, etc.). Could we follow that? https://www.delftstack.com/howto/java/java-utility-class.java/
--
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 diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843060971
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -287,11 +267,18 @@ public void setInstanceEnabled(boolean enabled) {
_record.setLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(),
System.currentTimeMillis());
if (enabled) {
- _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_REASON.toString());
- _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_TYPE.toString());
+ resetInstanceDisabledTypeAndReason();
}
}
+ /**
+ * Removes HELIX_DISABLED_REASON and HELIX_DISABLED_TYPE entry from simple field.
+ */
+ public void resetInstanceDisabledTypeAndReason() {
Review Comment:
TFTR. This will be used in ZkHelixManager in later PR.
--
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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843152388
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix;
Review Comment:
Have we considered putting this in `org.apache.helix.util` since this is a util class?
--
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] mgao0 commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
mgao0 commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843062756
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -287,11 +267,18 @@ public void setInstanceEnabled(boolean enabled) {
_record.setLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(),
System.currentTimeMillis());
if (enabled) {
- _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_REASON.toString());
- _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_TYPE.toString());
+ resetInstanceDisabledTypeAndReason();
}
}
+ /**
+ * Removes HELIX_DISABLED_REASON and HELIX_DISABLED_TYPE entry from simple field.
+ */
+ public void resetInstanceDisabledTypeAndReason() {
Review Comment:
Ok. Then let's leave it as is.
--
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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843165379
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public final class HelixConfigStringProcessUtil {
+ private static final String CONCATENATE_CONFIG_SPLITTER = ",";
+ private static final String CONCATENATE_CONFIG_JOINER = "=";
+
+ private HelixConfigStringProcessUtil() {
+ throw new java.lang.UnsupportedOperationException(
+ "Utility class HelixConfigStringProcessUtil and cannot be instantiated");
+ }
+
+ /**
+ * Parse a string represented map into a map.
+ * @param inputStr "propName0=propVal0,propName1=propVal1"
+ * @return map {[propName0, propVal0], [propName1, propVal1]}"
+ */
+ public static Map<String, String> concatenatedConfigParser(String inputStr) {
Review Comment:
Can we make the method name a camel case verb - for example, `parseDomainString()`?
--
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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843165855
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public final class HelixConfigStringProcessUtil {
+ private static final String CONCATENATE_CONFIG_SPLITTER = ",";
+ private static final String CONCATENATE_CONFIG_JOINER = "=";
+
+ private HelixConfigStringProcessUtil() {
+ throw new java.lang.UnsupportedOperationException(
+ "Utility class HelixConfigStringProcessUtil and cannot be instantiated");
+ }
+
+ /**
+ * Parse a string represented map into a map.
+ * @param inputStr "propName0=propVal0,propName1=propVal1"
+ * @return map {[propName0, propVal0], [propName1, propVal1]}"
+ */
+ public static Map<String, String> concatenatedConfigParser(String inputStr) {
+ Map<String, String> resultMap = new HashMap<>();
+ if (inputStr == null || inputStr.isEmpty()) {
+ return resultMap;
+ }
+ String[] pathPairs = inputStr.trim().split(CONCATENATE_CONFIG_SPLITTER);
+ for (String pair : pathPairs) {
+ String[] values = pair.split(CONCATENATE_CONFIG_JOINER);
+ if (values.length != 2 || values[0].isEmpty() || values[1].isEmpty()) {
+ throw new IllegalArgumentException(
+ String.format("Domain-Value pair %s is not valid.", pair));
+ }
+ resultMap.put(values[0].trim(), values[1].trim());
+ }
+ return resultMap;
+ }
+
+ /**
+ * Concatenate a map into a string .
+ * @param inputMap {[propName0, propVal0], [propName1, propVal1]}
+ * @return String "propName0=propVal0,propName1=propVal1"
+ */
+ public static String concatenateMapIntoString(Map<String, String> inputMap) {
Review Comment:
nit; `concatenateDomainMapping()`?
--
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] mgao0 commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
mgao0 commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843053284
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -287,11 +267,18 @@ public void setInstanceEnabled(boolean enabled) {
_record.setLongField(InstanceConfigProperty.HELIX_ENABLED_TIMESTAMP.name(),
System.currentTimeMillis());
if (enabled) {
- _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_REASON.toString());
- _record.getSimpleFields().remove(InstanceConfigProperty.HELIX_DISABLED_TYPE.toString());
+ resetInstanceDisabledTypeAndReason();
}
}
+ /**
+ * Removes HELIX_DISABLED_REASON and HELIX_DISABLED_TYPE entry from simple field.
+ */
+ public void resetInstanceDisabledTypeAndReason() {
Review Comment:
Can we make this a private method?
--
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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843156033
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public final class HelixConfigStringProcessUtil {
Review Comment:
Nit: when I see this class, the name suggests to me that this is a util class that processes all Helix utils... where in reality, I think you meant for this to be used only for parsing InstanceConfigs, and zooming further in, not all fields in InstanceConfig, but specifically the string that goes into the "domain" only.
As such, I think that it might improve readability and minimize confusion if you renamed it to something more descriptive - something like `InstanceDomainStringParser` or `InstanceDomainStringUtil`?
Lastly, a block comment above `public final class` that briefly describes what this does and why it's needed? (or more along the lines of what the domain in InstanceConfig is used for).
--
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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843156033
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public final class HelixConfigStringProcessUtil {
Review Comment:
Nit: when I see this class, the name suggests to me that this is a util class that processes all Helix utils... where in reality, I think you meant for this to be used only for parsing InstanceConfigs, and zooming further into this, not all fields in InstanceConfig, but specifically the string that goes into the "domain" only.
As such, I think that it might improve readability and minimize confusion if you renamed it to something more descriptive - something like `InstanceDomainStringParser` or `InstanceDomainStringUtil`?
Lastly, a block comment above `public final class` that briefly describes what this does and why it's needed? (or more along the lines of what the domain in InstanceConfig is used for).
--
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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843062536
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,64 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class HelixConfigStringProcessUtil {
Review Comment:
There is a commonly-followed Java idiom for creating util classes (`final` class, private constructor, etc.). Can we follow that? Some quick thing I found on Google: https://www.delftstack.com/howto/java/java-utility-class.java/
--
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 pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on PR #2015:
URL: https://github.com/apache/helix/pull/2015#issuecomment-1089005647
This change is ready to be merged. Approved by @qqu0127 //Thanks
Commit message:
Refactor common string processing util
--
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 diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843097646
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -327,7 +314,7 @@ public String getInstanceDisabledReason() {
*/
public String getInstanceDisabledType() {
if (getInstanceEnabled()) {
- return InstanceConstants.InstanceDisabledType.INSTANCE_NOT_DISABLED.toString();
+ return InstanceConstants.INSTANCE_NOT_DISABLED;
Review Comment:
Yes. It would be confusing for user to see "INSTANCE_NOT_DISABLED" in InstanceDisabledType. I use to a separate static String to represent not enabled instance.
--
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] narendly merged pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly merged PR #2015:
URL: https://github.com/apache/helix/pull/2015
--
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 diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843255542
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public final class HelixConfigStringProcessUtil {
Review Comment:
Thanks for the suggestion.
As our offline discussion, let's keep the class and method name more generic as it will be used for cluster config in following PR.
Changed the naming according to convention.
--
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 diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843253388
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,69 @@
+package org.apache.helix;
Review Comment:
Updated.
--
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 diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843115563
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,64 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class HelixConfigStringProcessUtil {
Review Comment:
TFTR. Updated
--
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 pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on PR #2015:
URL: https://github.com/apache/helix/pull/2015#issuecomment-1089149910
This change is ready to be merged. Approved by @qqu0127 @junkaixue //Thanks
Commit message:
Refactor common string processing util
--
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] junkaixue commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
junkaixue commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843080670
##########
helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java:
##########
@@ -327,7 +314,7 @@ public String getInstanceDisabledReason() {
*/
public String getInstanceDisabledType() {
if (getInstanceEnabled()) {
- return InstanceConstants.InstanceDisabledType.INSTANCE_NOT_DISABLED.toString();
+ return InstanceConstants.INSTANCE_NOT_DISABLED;
Review Comment:
So we changed this in last PR for string?
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,64 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class HelixConfigStringProcessUtil {
Review Comment:
This is a good suggestion. Let's follow this.
--
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] narendly commented on a diff in pull request #2015: Refactor common string processing util
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2015:
URL: https://github.com/apache/helix/pull/2015#discussion_r843062536
##########
helix-common/src/main/java/org/apache/helix/HelixConfigStringProcessUtil.java:
##########
@@ -0,0 +1,64 @@
+package org.apache.helix;
+
+/*
+ * 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.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class HelixConfigStringProcessUtil {
Review Comment:
There are commonly-followed Java idiom for creating util classes (`final` class, private constructor, etc.). Can we follow that? Some quick thing I found on Google: https://www.delftstack.com/howto/java/java-utility-class.java/
--
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