[GitHub] [helix] jiajunwang commented on a change in pull request #1579: Controller-side Task Current State Migration: other changes

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #1579:
URL: https://github.com/apache/helix/pull/1579#discussion_r539052092



##########
File path: helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/CurrentStatesResource.java
##########
@@ -65,10 +66,10 @@ StringRepresentation getInstanceCurrentStatesRepresentation(String clusterName,
     String instanceSessionId =
         ClusterRepresentationUtil.getInstanceSessionId(zkClient, clusterName, instanceName);
 
-    String message =
-        ClusterRepresentationUtil
-            .getInstancePropertyNameListAsString(zkClient, clusterName, instanceName,
-                PropertyType.CURRENTSTATES, instanceSessionId, MediaType.APPLICATION_JSON);
+    String message = ClusterRepresentationUtil
+        .getMultiInstancePropertyNameListsAsString(zkClient, clusterName, instanceName,

Review comment:
       This is fine, I guess you can just deprecate the older one.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKUtil.java
##########
@@ -153,6 +153,7 @@ public static boolean isInstanceSetup(RealmAwareZkClient zkclient, String cluste
       requiredPaths.add(PropertyPathBuilder.instanceConfig(clusterName, instanceName));
       requiredPaths.add(PropertyPathBuilder.instanceMessage(clusterName, instanceName));
       requiredPaths.add(PropertyPathBuilder.instanceCurrentState(clusterName, instanceName));
+      requiredPaths.add(PropertyPathBuilder.instanceTaskCurrentState(clusterName, instanceName));

Review comment:
       A new controller supports the older participants. So this should not be a required path, no?

##########
File path: helix-core/src/main/java/org/apache/helix/tools/commandtools/CurrentStateCleanUp.java
##########
@@ -124,15 +125,21 @@ public ZNRecord update(ZNRecord currentData) {
       LOG.info(String.format("Processing cleaning current state for instance: %s", instanceName));
       List<String> currentStateNames =
           accessor.getChildNames(accessor.keyBuilder().currentStates(instanceName, session));
-      for (String currentStateName : currentStateNames) {
-        PropertyKey key =
-            accessor.keyBuilder().currentState(instanceName, session, currentStateName);
+      List<String> taskCurrentStateNames =
+          accessor.getChildNames(accessor.keyBuilder().taskCurrentStates(instanceName, session));
+      List<PropertyKey> allCurrentStateKeys = new ArrayList<>();
+      currentStateNames.stream()
+          .map(name -> accessor.keyBuilder().currentState(instanceName, session, name))
+          .forEach(allCurrentStateKeys::add);
+      taskCurrentStateNames.stream()
+          .map(name -> accessor.keyBuilder().taskCurrentState(instanceName, session, name))
+          .forEach(allCurrentStateKeys::add);
+      for (PropertyKey key : allCurrentStateKeys) {
         accessor.getBaseDataAccessor().update(key.getPath(), updater, AccessOption.PERSISTENT);
         CurrentState currentState = accessor.getProperty(key);
         if (currentState.getPartitionStateMap().size() == 0) {
           accessor.getBaseDataAccessor().remove(key.getPath(), AccessOption.PERSISTENT);

Review comment:
       Can we change to use the multi remove method that is also supported by the accessor?

##########
File path: helix-core/src/main/java/org/apache/helix/util/InstanceValidationUtil.java
##########
@@ -104,9 +104,8 @@ public static boolean hasResourceAssigned(HelixDataAccessor dataAccessor, String
     if (liveInstance != null) {
       String sessionId = liveInstance.getEphemeralOwner();
 
-      List<String> resourceNames = dataAccessor.getChildNames(propertyKeyBuilder.currentStates(instanceName, sessionId));
-      for (String resourceName : resourceNames) {
-        PropertyKey currentStateKey = propertyKeyBuilder.currentState(instanceName, sessionId, resourceName);
+      for (PropertyKey currentStateKey : getAllCurrentStateKeys(dataAccessor, instanceName,

Review comment:
       nit, could you also update the method comment?
   
   "at least 1 resource" -> "at least 1 resource or task", something like 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.

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