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/01 21:30:25 UTC
[GitHub] [helix] mgao0 commented on a change in pull request #2006: Add type and reason for batch disable/enable
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