[GitHub] [helix] jiajunwang commented on a change in pull request #1620: Skeleton per-replica stage with one simple test (1st)

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

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



##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/AttributeName.java
##########
@@ -50,5 +50,11 @@
   // This attribute should only be used in TaskGarbageCollectionStage, misuse could cause race conditions.
   JOBS_WITHOUT_CONFIG,
   // This attribute should only be used in TaskGarbageCollectionStage, misuse could cause race conditions.
-  TO_BE_PURGED_JOBS_MAP
+  TO_BE_PURGED_JOBS_MAP,
+
+  // This attribute denotes the messages output from Per Preplica Throttle stage
+  PER_REPLICA_OUTPUT_MESSAGES,
+
+  // This attribute denotes the targeted partition state mapping from Per Preplica Throttle stage
+  PER_REPLICA_RETRACED_STATES

Review comment:
       It is a question instead of a change request. Shall we just reuse INTERMEDIATE_STATE?
   
   So here are 2 options,
   1. We keep the old behavior, and add a configuration to switch. In this case, we need to keep INTERMEDIATE_STATE and add this new one.
   2. We replace the INTERMEDIATE stage with the new logic. Then we don't need to add the new field. We can just reuse INTERMEDIATE_STATE.

##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/MessageOutput.java
##########
@@ -24,6 +24,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;

Review comment:
       Please remove unnecessary change.

##########
File path: helix-core/src/test/java/org/apache/helix/controller/stages/MsgRecordingPerReplicaThrottleStage.java
##########
@@ -0,0 +1,58 @@
+package org.apache.helix.controller.stages;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.helix.api.config.StateTransitionThrottleConfig;
+import org.apache.helix.controller.dataproviders.ResourceControllerDataProvider;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Partition;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class MsgRecordingPerReplicaThrottleStage extends PerReplicaThrottleStage {

Review comment:
       Any comment about what is this class for? Why we need a child class of PerReplicaThrottleStage instead of just adding the logic there? 

##########
File path: helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ResourceMonitor.java
##########
@@ -47,7 +47,8 @@
     UNKNOWN,
     NORMAL,
     BEST_POSSIBLE_STATE_CAL_FAILED,
-    INTERMEDIATE_STATE_CAL_FAILED
+    INTERMEDIATE_STATE_CAL_FAILED,
+    PER_REPLICA_STATE_CAL_FAILED

Review comment:
       nit, the name should fit the stage name, PER_REPLICA_THROTTLE_CAL_FAILED




----------------------------------------------------------------
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