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/03/31 23:43:08 UTC
[GitHub] [helix] xyuanlu opened a new pull request #2006: Add type and reason for batch disable/enable
xyuanlu opened a new pull request #2006:
URL: https://github.com/apache/helix/pull/2006
### 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:
Having a batching enable/disable would reduce number of ZK write. It is also helps user to solve some race conditions as all instance enabling/disabling happens at one ZNode.
Helix already have a batch instance enable/disable feature but it is inactive now. This PR active this feature as the dependency is resolved. Helix should also allow user to set disabled type and reason for these instances.
### Tests
- [X] The following tests are written for this issue:
Enabled :
TestBatchEnableInstances
Extended :
TestBatchEnableInstances.testBatchDisableOldEnable
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.)
- 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] mgao0 commented on a change in pull request #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840913708
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
+ private static final String CONCATENATE_CONFIG_SPLITTER = ",";
+ private static final String CONCATENATE_CONFIG_JOINER = "=";
+
+
+ public static Map<String, String> concatenateConfigParser(String inputStr) {
Review comment:
Could you add some java doc comments for this util method, an example would be very helpful. Such as input "propName0=propVal0,propName1=propVal1" => output map {[propName0, propVal0], [propName1, propVal1]}
nit: "concatenate_d_ConfigParser"
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
+ private static final String CONCATENATE_CONFIG_SPLITTER = ",";
Review comment:
Let's evaluate future use cases, will "," always be the splitter and "=" always be the joiner? If not, maybe we can make it configurable.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -352,27 +353,28 @@ public void enableInstance(final String clusterName, final String instanceName,
clusterName);
BaseDataAccessor<ZNRecord> baseAccessor = new ZkBaseDataAccessor<>(_zkClient);
enableSingleInstance(clusterName, instanceName, enabled, baseAccessor, disabledType, reason);
- // TODO: Reenable this after storage node bug fixed.
- // enableBatchInstances(clusterName, Collections.singletonList(instanceName), enabled, baseAccessor);
+ enableBatchInstances(clusterName, Collections.singletonList(instanceName), enabled,
+ baseAccessor, disabledType, reason);
}
@Override
- public void enableInstance(String clusterName, List<String> instances, boolean enabled) {
- // TODO: Considering adding another batched API with disabled type and reason.
- // TODO: Reenable this after storage node bug fixed.
- if (true) {
- throw new HelixException("Current batch enable/disable instances are temporarily disabled!");
- }
+ public void enableInstance(String clusterName, List<String> instances, boolean enabled,
+ InstanceConstants.InstanceDisabledType disabledType, String reason) {
logger.info("Batch {} instances {} in cluster {}.", enabled ? "enable" : "disable",
HelixUtil.serializeByComma(instances), clusterName);
BaseDataAccessor<ZNRecord> baseAccessor = new ZkBaseDataAccessor<>(_zkClient);
if (enabled) {
for (String instance : instances) {
- enableSingleInstance(clusterName, instance, enabled, baseAccessor, null, null);
+ enableSingleInstance(clusterName, instance, enabled, baseAccessor, disabledType, reason);
Review comment:
Same here, add some comments
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -352,27 +353,28 @@ public void enableInstance(final String clusterName, final String instanceName,
clusterName);
BaseDataAccessor<ZNRecord> baseAccessor = new ZkBaseDataAccessor<>(_zkClient);
enableSingleInstance(clusterName, instanceName, enabled, baseAccessor, disabledType, reason);
Review comment:
Shall we add some comments here saying "right now is transition period so both single and batch are enabled, should be removed once migration is completed balabala"?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -134,7 +136,13 @@
// offline for more than this specified time period, and users call purge participant API,
// then the node will be removed.
// The unit is milliseconds.
- OFFLINE_DURATION_FOR_PURGE_MS
+ OFFLINE_DURATION_FOR_PURGE_MS,
+
+ // The following 3 keywords are for metadata in batch disabled instance
+ HELIX_ENABLED_TIMESTAMP,
Review comment:
I'm thinking if this will be confusing for some users. `HELIX_ENABLED_DISABLED_TIMESTAMP` might be better?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java
##########
@@ -142,7 +143,9 @@ private static long getInactiveTime(String instance, Set<String> liveInstances,
if (clusterConfig.getDisabledInstances() != null && clusterConfig.getDisabledInstances()
.containsKey(instance)) {
// Update batch disable time
- long batchDisableTime = Long.parseLong(clusterConfig.getDisabledInstances().get(instance));
+ long batchDisableTime = Long.parseLong(
Review comment:
Could parseLong() be called on `null`?
--
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 #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840967444
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
Review comment:
Test of these 2 functions are covered in tests of their caller. I think no dedicated test is needed.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1927,14 +1933,30 @@ public ZNRecord update(ZNRecord currentData) {
if (clusterConfig.getDisabledInstances() != null) {
disabledInstances.putAll(clusterConfig.getDisabledInstances());
}
-
if (enabled) {
disabledInstances.keySet().removeAll(instances);
} else {
for (String disabledInstance : instances) {
- if (!disabledInstances.containsKey(disabledInstance)) {
- disabledInstances.put(disabledInstance, String.valueOf(System.currentTimeMillis()));
+ Map<String, String> disableInfo = new TreeMap<>();
+ disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_ENABLED_TIMESTAMP.toString(),
+ String.valueOf(System.currentTimeMillis()));
+ if (disabledType != null) {
+ if (disabledType
+ .equals(InstanceConstants.InstanceDisabledType.INSTANCE_NOT_DISABLED)) {
+ throw new HelixException(
+ "Can not set INSTANCE_NOT_DISABLED as disabled type to an instance");
+ }
+ disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_TYPE.toString(),
+ disabledType.toString());
+ }
+ if (reason != null) {
+ disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_REASON.toString(),
+ reason);
}
+ // We allow user to override disabledType and reason for an already disabled instance.
+ // TODO: update the history ZNode
+ disabledInstances
+ .put(disabledInstance, StringProcessUtil.concatenateMapIntoString(disableInfo));
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] qqu0127 commented on a change in pull request #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
qqu0127 commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840940478
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
Review comment:
Apache license?
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
Review comment:
Depending on the use case, StringProcess is too generic a name, and right now this class seems specific to parsing config. Shall we consider rename?
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
Review comment:
Also let's add unit test for it, thanks.
##########
File path: helix-core/src/test/java/org/apache/helix/mock/MockHelixAdmin.java
##########
@@ -295,16 +302,75 @@ public void enableInstance(String clusterName, String instanceName, boolean enab
_baseDataAccessor.set(instanceConfigPath, instanceConfig.getRecord(), 0);
}
- @Override public void enableInstance(String clusterName, List<String> instances,
- boolean enabled) {
+ @Override
+ public void enableInstance(String clusterName, List<String> instances, boolean enabled) {
+ enableInstance(clusterName, instances, enabled, null, null);
+ }
+
+ @Override
+ public void enableInstance(String clusterName, List<String> instances, boolean enabled,
+ InstanceConstants.InstanceDisabledType disabledType, String reason) {
+
+ String path = PropertyPathBuilder.clusterConfig(clusterName);
+
+ if (!_baseDataAccessor.exists(path, 0)) {
+ _baseDataAccessor.create(path, new ZNRecord(clusterName), 0);
+ }
+
+ _baseDataAccessor.update(path, new DataUpdater<ZNRecord>() {
+ @Override
+ public ZNRecord update(ZNRecord currentData) {
Review comment:
Let's try to reduce code copy-paste
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -1927,14 +1933,30 @@ public ZNRecord update(ZNRecord currentData) {
if (clusterConfig.getDisabledInstances() != null) {
disabledInstances.putAll(clusterConfig.getDisabledInstances());
}
-
if (enabled) {
disabledInstances.keySet().removeAll(instances);
} else {
for (String disabledInstance : instances) {
- if (!disabledInstances.containsKey(disabledInstance)) {
- disabledInstances.put(disabledInstance, String.valueOf(System.currentTimeMillis()));
+ Map<String, String> disableInfo = new TreeMap<>();
+ disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_ENABLED_TIMESTAMP.toString(),
+ String.valueOf(System.currentTimeMillis()));
+ if (disabledType != null) {
+ if (disabledType
+ .equals(InstanceConstants.InstanceDisabledType.INSTANCE_NOT_DISABLED)) {
+ throw new HelixException(
+ "Can not set INSTANCE_NOT_DISABLED as disabled type to an instance");
+ }
+ disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_TYPE.toString(),
+ disabledType.toString());
+ }
+ if (reason != null) {
+ disableInfo.put(ClusterConfig.ClusterConfigProperty.HELIX_DISABLED_REASON.toString(),
+ reason);
}
+ // We allow user to override disabledType and reason for an already disabled instance.
+ // TODO: update the history ZNode
+ disabledInstances
+ .put(disabledInstance, StringProcessUtil.concatenateMapIntoString(disableInfo));
Review comment:
nit: this is a little hard to read, can we make these a separate method?
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1045,4 +1053,28 @@ public int hashCode() {
public String getClusterName() {
return _record.getId();
}
+
+ public String getPlainInstanceHelixDisabledType(String instanceName) {
+ return StringProcessUtil.concatenateConfigParser(getDisabledInstances().get(instanceName))
+ .get(ClusterConfigProperty.HELIX_DISABLED_TYPE.toString());
+ }
Review comment:
Open discussion, can this and below methods be put in the util class instead? I think this ClusterConfig class is getting bloated.
--
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 #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840950298
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
+ private static final String CONCATENATE_CONFIG_SPLITTER = ",";
Review comment:
TFTR. Right now the two existing use cases all use "," and "=". I think it is OK to hard code it for now. It is also pretty standard to represent map this way.
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
+ private static final String CONCATENATE_CONFIG_SPLITTER = ",";
Review comment:
TFTR. Right now the two existing use cases all use "," and "=". I think it is OK to hard code it for now. It is also pretty standard to serialize map this way.
--
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 #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840950298
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
+ private static final String CONCATENATE_CONFIG_SPLITTER = ",";
Review comment:
TFTR. Right now the two existing use cases all use "," and "=". I think it is OK to hard code it for now.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
##########
@@ -352,27 +353,28 @@ public void enableInstance(final String clusterName, final String instanceName,
clusterName);
BaseDataAccessor<ZNRecord> baseAccessor = new ZkBaseDataAccessor<>(_zkClient);
enableSingleInstance(clusterName, instanceName, enabled, baseAccessor, disabledType, reason);
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 a change in pull request #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840951235
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/DelayedRebalanceUtil.java
##########
@@ -142,7 +143,9 @@ private static long getInactiveTime(String instance, Set<String> liveInstances,
if (clusterConfig.getDisabledInstances() != null && clusterConfig.getDisabledInstances()
.containsKey(instance)) {
// Update batch disable time
- long batchDisableTime = Long.parseLong(clusterConfig.getDisabledInstances().get(instance));
+ long batchDisableTime = Long.parseLong(
Review comment:
We always add the Timestamp when batch enabling instance. I don't think it could be null unless ZNode is manually 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 change in pull request #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840951316
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -134,7 +136,13 @@
// offline for more than this specified time period, and users call purge participant API,
// then the node will be removed.
// The unit is milliseconds.
- OFFLINE_DURATION_FOR_PURGE_MS
+ OFFLINE_DURATION_FOR_PURGE_MS,
+
+ // The following 3 keywords are for metadata in batch disabled instance
+ HELIX_ENABLED_TIMESTAMP,
Review comment:
Good point, 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 change in pull request #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840974483
##########
File path: helix-core/src/main/java/org/apache/helix/model/ClusterConfig.java
##########
@@ -1045,4 +1053,28 @@ public int hashCode() {
public String getClusterName() {
return _record.getId();
}
+
+ public String getPlainInstanceHelixDisabledType(String instanceName) {
+ return StringProcessUtil.concatenateConfigParser(getDisabledInstances().get(instanceName))
+ .get(ClusterConfigProperty.HELIX_DISABLED_TYPE.toString());
+ }
Review comment:
I think a getter for properties stored in clusterConfig is OK to be put into this class. Parser or other processor should be moved to 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 change in pull request #2006: Add type and reason for batch disable/enable
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #2006:
URL: https://github.com/apache/helix/pull/2006#discussion_r840965655
##########
File path: helix-common/src/main/java/org/apache/helix/StringProcessUtil.java
##########
@@ -0,0 +1,38 @@
+package org.apache.helix;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+
+public class StringProcessUtil {
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