junkaixue commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1568041373
##########
helix-core/src/main/java/org/apache/helix/HelixAdmin.java:
##########
@@ -415,6 +415,17 @@ void manuallyEnableMaintenanceMode(String clusterName,
boolean enabled, String r
*/
ClusterManagementMode getClusterManagementMode(String clusterName);
+ /**
+ * Set a list of partitions for an instance to ERROR from any current state.
+ * The partitions could be in any state and setPartitionToError will bring
them to ERROR
+ * from their current state. A CURRENT_STATE to ERROR state transition is
required for this.
+ * @param clusterName
+ * @param instanceName
+ * @param resourceName
+ * @param partitionNames
+ */
+ void setPartitionToError(String clusterName, String instanceName, String
resourceName, List<String> partitionNames);
Review Comment:
+1
##########
helix-core/src/main/java/org/apache/helix/HelixAdmin.java:
##########
@@ -415,6 +415,17 @@ void manuallyEnableMaintenanceMode(String clusterName,
boolean enabled, String r
*/
ClusterManagementMode getClusterManagementMode(String clusterName);
+ /**
+ * Set a list of partitions for an instance to ERROR from any current state.
+ * The partitions could be in any state and setPartitionToError will bring
them to ERROR
+ * from their current state. A CURRENT_STATE to ERROR state transition is
required for this.
+ * @param clusterName
+ * @param instanceName
+ * @param resourceName
+ * @param partitionNames
+ */
+ void setPartitionToError(String clusterName, String instanceName, String
resourceName, List<String> partitionNames);
Review Comment:
We don't need to use current state, the from state can be "*" and it will be
more easy for user implementation.
##########
helix-core/src/main/java/org/apache/helix/HelixAdmin.java:
##########
@@ -516,8 +527,7 @@ void addStateModelDef(String clusterName, String
stateModelDef, StateModelDefini
* @param resourceName
* @return customized for the resource
*/
- CustomizedView getResourceCustomizedView(String clusterName, String
resourceName,
- String customizedStateType);
+ CustomizedView getResourceCustomizedView(String clusterName, String
resourceName, String customizedStateType);
Review Comment:
Do you set the right style formatter? Helix code has its own formatter in
open source code repo
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:
##########
@@ -1085,6 +1085,139 @@ public ClusterManagementMode
getClusterManagementMode(String clusterName) {
: new ClusterManagementMode(status.getManagementMode(),
status.getManagementModeStatus());
}
+ private enum SetPartitionToErrorFailureReason {
+ INSTANCE_NOT_ALIVE("%s is not alive in cluster %s"),
+ INSTANCE_NON_EXISTENT("%s does not exist in cluster %s"),
+ RESOURCE_NON_EXISTENT("resource %s is not added to cluster %s"),
+ PARTITION_NON_EXISTENT("not all %s exist in cluster %s"),
+ PARTITION_ALREADY_IN_ERROR("%s is NOT found in cluster %s or is already in
ERROR state"),
+ STATE_MODEL_NON_EXISTENT("%s is NOT found in cluster %s");
+
+ private String message;
+
+ SetPartitionToErrorFailureReason(String message) {
+ this.message = message;
+ }
+
+ public String getMessage(String resourceName, List<String> partitionNames,
String instanceName,
+ String errorStateEntity, String clusterName) {
+ return String.format("Can't set to Error State for %s.%s on %s, because
" + message, resourceName,
+ partitionNames, instanceName, errorStateEntity, clusterName);
+ }
+ }
+
+ @Override
+ public void setPartitionToError(String clusterName, String instanceName,
String resourceName,
+ List<String> partitionNames) {
+ logger.info("Set partitions {} for resource {} on instance {} in cluster
{} to ERROR state.",
+ partitionNames == null ? "NULL" :
HelixUtil.serializeByComma(partitionNames), resourceName, instanceName,
+ clusterName);
+ HelixDataAccessor accessor = new ZKHelixDataAccessor(clusterName, new
ZkBaseDataAccessor<ZNRecord>(_zkClient));
+ PropertyKey.Builder keyBuilder = accessor.keyBuilder();
+
+ // check the instance is alive
+ LiveInstance liveInstance =
accessor.getProperty(keyBuilder.liveInstance(instanceName));
+ if (liveInstance == null) {
+ // check if the instance exists in the cluster
+ String instanceConfigPath =
PropertyPathBuilder.instanceConfig(clusterName, instanceName);
+ throw new HelixException(String.format(
+ (_zkClient.exists(instanceConfigPath) ?
SetPartitionToErrorFailureReason.INSTANCE_NOT_ALIVE
+ :
SetPartitionToErrorFailureReason.INSTANCE_NON_EXISTENT).getMessage(resourceName,
partitionNames,
+ instanceName, instanceName, clusterName)));
+ }
+
+ // check resource exists in ideal state
+ IdealState idealState =
accessor.getProperty(keyBuilder.idealStates(resourceName));
+ if (idealState == null) {
+ throw new
HelixException(String.format(SetPartitionToErrorFailureReason.RESOURCE_NON_EXISTENT
+ .getMessage(resourceName, partitionNames, instanceName,
resourceName, clusterName)));
+ }
+
+ // check partition exists in resource
+ Set<String> partitionsToBeSetToError = new HashSet<String>(partitionNames);
+ Set<String> partitions =
+ (idealState.getRebalanceMode() == RebalanceMode.CUSTOMIZED) ?
idealState.getRecord()
+ .getMapFields().keySet() :
idealState.getRecord().getListFields().keySet();
+ if (!partitions.containsAll(partitionsToBeSetToError)) {
+ throw new
HelixException(String.format(SetPartitionToErrorFailureReason.PARTITION_NON_EXISTENT
+ .getMessage(resourceName, partitionNames, instanceName,
partitionNames.toString(),
+ clusterName)));
+ }
+
+ // check partition is not already in ERROR state
+ String sessionId = liveInstance.getEphemeralOwner();
Review Comment:
We don't have to make a session for that. "*" is fair enough.
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:
##########
@@ -1085,6 +1085,139 @@ public ClusterManagementMode
getClusterManagementMode(String clusterName) {
: new ClusterManagementMode(status.getManagementMode(),
status.getManagementModeStatus());
}
+ private enum SetPartitionToErrorFailureReason {
+ INSTANCE_NOT_ALIVE("%s is not alive in cluster %s"),
+ INSTANCE_NON_EXISTENT("%s does not exist in cluster %s"),
+ RESOURCE_NON_EXISTENT("resource %s is not added to cluster %s"),
+ PARTITION_NON_EXISTENT("not all %s exist in cluster %s"),
+ PARTITION_ALREADY_IN_ERROR("%s is NOT found in cluster %s or is already in
ERROR state"),
+ STATE_MODEL_NON_EXISTENT("%s is NOT found in cluster %s");
+
+ private String message;
+
+ SetPartitionToErrorFailureReason(String message) {
+ this.message = message;
+ }
+
+ public String getMessage(String resourceName, List<String> partitionNames,
String instanceName,
+ String errorStateEntity, String clusterName) {
+ return String.format("Can't set to Error State for %s.%s on %s, because
" + message, resourceName,
+ partitionNames, instanceName, errorStateEntity, clusterName);
+ }
+ }
+
+ @Override
+ public void setPartitionToError(String clusterName, String instanceName,
String resourceName,
Review Comment:
+1. Please refactor resetPartition if it is needed. Let's leverage the code
that already exists.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
