You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "csudharsanan (via GitHub)" <gi...@apache.org> on 2024/04/16 21:16:58 UTC
[PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
csudharsanan opened a new pull request, #2792:
URL: https://github.com/apache/helix/pull/2792
### Issues
Fixes #2791
### Description
What: An API endpoint that validates the incoming request and sends a state transition message to sets one or more partitions from any current state to ERROR state.
Why: Currently, the participants are unable to set a partition to an ERROR state explicitly when they seem to be stuck in a specific current state. The only way a replica can be set to ERROR is from within a state model. Having an endpoint to allow this behavior would allow the clients to call the resetPartition endpoint to set it back to INIT state and recover the replica. resetPartition works only on partitions in error state.
### Tests
```
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache Helix 1.3.2-SNAPSHOT:
[INFO]
[INFO] Apache Helix ....................................... SUCCESS [ 1.504 s]
[INFO] Apache Helix :: Metrics Common ..................... SUCCESS [ 0.244 s]
[INFO] Apache Helix :: Metadata Store Directory Common .... SUCCESS [ 0.363 s]
[INFO] Apache Helix :: ZooKeeper API ...................... SUCCESS [ 0.380 s]
[INFO] Apache Helix :: Helix Common ....................... SUCCESS [ 0.291 s]
[INFO] Apache Helix :: Core ............................... SUCCESS [ 0.306 s]
[INFO] Apache Helix :: Admin Webapp ....................... SUCCESS [ 0.606 s]
[INFO] Apache Helix :: Restful Interface .................. SUCCESS [ 0.941 s]
[INFO] Apache Helix :: Distributed Lock ................... SUCCESS [ 0.228 s]
[INFO] Apache Helix :: HelixAgent ......................... SUCCESS [ 0.187 s]
[INFO] Apache Helix :: Recipes ............................ SUCCESS [ 0.033 s]
[INFO] Apache Helix :: Recipes :: Rabbitmq Consumer Group . SUCCESS [ 0.205 s]
[INFO] Apache Helix :: Recipes :: Rsync Replicated File Store SUCCESS [ 0.248 s]
[INFO] Apache Helix :: Recipes :: distributed lock manager SUCCESS [ 0.169 s]
[INFO] Apache Helix :: Recipes :: distributed task execution SUCCESS [ 0.246 s]
[INFO] Apache Helix :: Recipes :: service discovery ....... SUCCESS [ 0.186 s]
[INFO] Apache Helix :: View Aggregator .................... SUCCESS [ 0.167 s]
[INFO] Apache Helix :: Meta Client ........................ SUCCESS [ 0.146 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 9.219 s
[INFO] Finished at: 2024-04-16T13:13:02-07:00
[INFO] ------------------------------------------------------------------------
```
- [ ] The following tests are written/updated for this issue:
- TestSetPartitionToErrorState (Integration)
- TestZkHelixAdmin : testSetPartitionToError
- TestPerInstanceAccessor : updateInstance
mvn test -o -Dtest=TestSetPartitionToErrorState -pl=helix-core
AfterClass: TestSetPartitionToErrorState called.
Shut down zookeeper at port 2183 in thread main
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 17.697 s - in org.apache.helix.integration.TestSetPartitionToErrorState
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco:0.8.6:report (generate-code-coverage-report) @ helix-core ---
[INFO] Loading execution data file /Users/csudhars/Apr15Helix-ForkedHelix/helix/helix-core/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Core' with 806 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 59.426 s
[INFO] Finished at: 2024-04-16T14:14:54-07:00
[INFO] ------------------------------------------------------------------------
mvn test -o -Dtest=TestZkHelixAdmin -pl=helix-core
END testZkHelixAdmin at Tue Apr 16 12:57:29 PDT 2024
AfterClass: TestZkHelixAdmin called.
Shut down zookeeper at port 2183 in thread main
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 66.246 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco:0.8.6:report (generate-code-coverage-report) @ helix-core ---
[INFO] Loading execution data file /Users/csudhars/Apr15Helix-ForkedHelix/helix/helix-core/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Core' with 951 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 02:00 min
[INFO] Finished at: 2024-04-16T12:57:38-07:00
[INFO] -
mvn test -o -Dtest=TestPerInstanceAccessor -pl=helix-rest
[INFO] Tests run: 24, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 219.352 s - in org.apache.helix.rest.server.TestPerInstanceAccessor
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 24, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco:0.8.6:report (generate-code-coverage-report) @ helix-rest ---
[INFO] Loading execution data file /Users/csudhars/Apr15Helix-ForkedHelix/helix/helix-rest/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Restful Interface' with 95 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:52 min
[INFO] Finished at: 2024-04-16T13:03:56-07:00
[INFO] -
### 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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1586842047
##########
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 =
Review Comment:
from what i understand, the RebalanceMode used is independent of the state model and the state transitions. Since we are sending a state transition message directly to the participant in these two cases - reset and setPartitionsToError, we can allow it for all modes.
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1591674565
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:
##########
@@ -1035,6 +1035,135 @@ public ClusterManagementMode getClusterManagementMode(String clusterName) {
: new ClusterManagementMode(status.getManagementMode(), status.getManagementModeStatus());
}
+ @Override
+ public void setPartitionsToError(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);
+ sendStateTransitionMessage(clusterName, instanceName, resourceName, partitionNames,
+ Boolean.FALSE);
Review Comment:
sounds good, 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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1586834604
##########
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:
updated the PR
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on PR #2792:
URL: https://github.com/apache/helix/pull/2792#issuecomment-2099213160
Fixed the mbean issue in HelixTask. Now it supports * -> * transitions. Since this wasn't failing tests, adding some logs.
Before:
```
Start zookeeper at localhost:2183 in thread main
START TestSetPartitionsToErrorState_testSetPartitionsToErrorState at Tue May 07 12:28:02 PDT 2024
true: wait 332ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
javax.management.RuntimeOperationsException
at java.management/com.sun.jmx.mbeanserver.Repository.addMBean(Repository.java:298)
at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerWithRepository(DefaultMBeanServerInterceptor.java:1848)
at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerDynamicMBean(DefaultMBeanServerInterceptor.java:945)
at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerObject(DefaultMBeanServerInterceptor.java:880)
at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerMBean(DefaultMBeanServerInterceptor.java:315)
at java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.registerMBean(JmxMBeanServer.java:523)
at org.apache.helix.monitoring.mbeans.MBeanRegistrar.register(MBeanRegistrar.java:60)
at org.apache.helix.monitoring.mbeans.dynamicMBeans.DynamicMBeanProvider.doRegister(DynamicMBeanProvider.java:89)
at org.apache.helix.monitoring.mbeans.dynamicMBeans.DynamicMBeanProvider.doRegister(DynamicMBeanProvider.java:95)
at org.apache.helix.monitoring.mbeans.StateTransitionStatMonitor.register(StateTransitionStatMonitor.java:83)
at org.apache.helix.monitoring.mbeans.ParticipantStatusMonitor.reportTransitionStat(ParticipantStatusMonitor.java:113)
at org.apache.helix.messaging.handling.HelixTask.reportMessageStat(HelixTask.java:335)
at org.apache.helix.messaging.handling.HelixTask.finalCleanup(HelixTask.java:386)
at org.apache.helix.messaging.handling.HelixTask.call(HelixTask.java:185)
at org.apache.helix.messaging.handling.HelixTask.call(HelixTask.java:49)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.lang.IllegalArgumentException: Repository: cannot add mbean for pattern name CLMParticipantReport:Cluster=TestSetPartitionsToErrorState_testSetPartitionsToErrorState,Transition=*--ERROR
... 19 more
true: wait 233ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
true: wait 216ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
16468 [ZkClient-EventThread-162-localhost:2183] ERROR org.apache.helix.messaging.handling.HelixTaskExecutor [] - Message xyz cannot be processed: ***, {CREATE_TIMESTAMP=1715110092791, FROM_STATE=*, MSG_ID=***, MSG_STATE=new, MSG_TYPE=STATE_TRANSITION, PARTITION_NAME=TestDB0_7, RESOURCE_NAME=TestDB0, SRC_NAME=*****, STATE_MODEL_DEF=MasterSlave, STATE_MODEL_FACTORY_NAME=DEFAULT, TGT_NAME=localhost_12918, TGT_SESSION_ID=***, TO_STATE=ERROR}{}{}Partition TestDB0_7 current state is same as toState (*->ERROR) from message.
true: wait 53ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
END TestSetPartitionsToErrorState_testSetPartitionsToErrorState at Tue May 07 12:28:15 PDT 2024
```
After:
```
Start zookeeper at localhost:2183 in thread main
START TestSetPartitionsToErrorState_testSetPartitionsToErrorState at Tue May 07 12:23:24 PDT 2024
true: wait 302ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
true: wait 202ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
true: wait 185ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
16489 [ZkClient-EventThread-162-localhost:2183] ERROR org.apache.helix.messaging.handling.HelixTaskExecutor [] - Message xyz cannot be processed: ***, {CREATE_TIMESTAMP=1715109814097, FROM_STATE=*, MSG_ID=***, MSG_STATE=new, MSG_TYPE=STATE_TRANSITION, PARTITION_NAME=TestDB0_7, RESOURCE_NAME=TestDB0, SRC_NAME=*****, STATE_MODEL_DEF=MasterSlave, STATE_MODEL_FACTORY_NAME=DEFAULT, TGT_NAME=localhost_12918, TGT_SESSION_ID=***, TO_STATE=ERROR}{}{}Partition TestDB0_7 current state is same as toState (*->ERROR) from message.
true: wait 51ms, ClusterStateVerifier$BestPossAndExtViewZkVerifier(TestSetPartitionsToErrorState_testSetPartitionsToErrorState@localhost:2183)
END TestSetPartitionsToErrorState_testSetPartitionsToErrorState at Tue May 07 12:23:36 PDT 2024
AfterClass: TestSetPartitionsToErrorState called.
Shut down zookeeper at port 2183 in thread main
```
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1586837657
##########
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:
it is public. Updated ZKHelixAdmin to reflect that, and renamed the method to setPartitionsToError
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue merged PR #2792:
URL: https://github.com/apache/helix/pull/2792
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1591337211
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:
##########
@@ -1035,6 +1035,135 @@ public ClusterManagementMode getClusterManagementMode(String clusterName) {
: new ClusterManagementMode(status.getManagementMode(), status.getManagementModeStatus());
}
+ @Override
+ public void setPartitionsToError(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);
+ sendStateTransitionMessage(clusterName, instanceName, resourceName, partitionNames,
+ Boolean.FALSE);
Review Comment:
Let's make it more generic. you can create an enum for actions like "reset, error". This function can be leveraged more in the future.
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1586835031
##########
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:
changed the formatter
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1567959515
##########
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:
is it private method?
nit: since it is taking list of partitionNames, should be called `setPartitionsToError` ?
##########
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:
the code is lot common with 'resetPartition' - can the common code be separated out and add only the deltas ?
##########
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 =
Review Comment:
for CUSTOMIZED_MODE - we shouldn't be allowing this, correct?
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on PR #2792:
URL: https://github.com/apache/helix/pull/2792#issuecomment-2099304436
This PR is ready to be merged
--
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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1591674425
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:
##########
@@ -1035,6 +1035,135 @@ public ClusterManagementMode getClusterManagementMode(String clusterName) {
: new ClusterManagementMode(status.getManagementMode(), status.getManagementModeStatus());
}
+ @Override
+ public void setPartitionsToError(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);
+ sendStateTransitionMessage(clusterName, instanceName, resourceName, partitionNames,
+ Boolean.FALSE);
Review Comment:
makes sense, 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
Re: [PR] [apache/helix] -- Add SetPartitionToError for participants to self annotate a node to ERROR state [helix]
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2792:
URL: https://github.com/apache/helix/pull/2792#discussion_r1591354567
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:
##########
@@ -1035,6 +1035,135 @@ public ClusterManagementMode getClusterManagementMode(String clusterName) {
: new ClusterManagementMode(status.getManagementMode(), status.getManagementModeStatus());
}
+ @Override
+ public void setPartitionsToError(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);
+ sendStateTransitionMessage(clusterName, instanceName, resourceName, partitionNames,
+ Boolean.FALSE);
Review Comment:
reset = false, means error seems a bit wrong. Can we introduce Enum with 3 values - ERROR, RESET, UNDEFINED
Pass ERROR and RESET and process it, for undefined, throw exception.
--
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