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 2020/12/09 19:19:47 UTC
[GitHub] [helix] NealSun96 opened a new pull request #1584: Participant-side Task Current State Migration
NealSun96 opened a new pull request #1584:
URL: https://github.com/apache/helix/pull/1584
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1549 (Partially)
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
To address excessive ZooKeeper reads caused by task framework CurrentState updates, it is proposed to move task framework CurrentStates to their separate path.
The change is divided to multiple phases. This is the second phase, where the main focus of the changes are on the participant side. A system property flag is created to disable this feature when necessary.
### Tests
- [x] The following tests are written for this issue:
testCurrentStatesRoutingTableIgnoreTaskCurrentStates
- [x] The following is the result of the "mvn test" command on the appropriate module:
```
[INFO] Tests run: 1251, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5,082.552 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1251, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:24 h
[INFO] Finished at: 2020-12-09T10:52:19-08:00
[INFO] ------------------------------------------------------------------------
```
### 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.
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r542931895
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -84,4 +84,7 @@
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
public static final String STATEUPDATEUTIL_ERROR_PERSISTENCY_ENABLED = "helix.StateUpdateUtil.errorLog.enabled";
+
+ public static final String TASK_CURRENT_STATE_PATH_DISABLED =
Review comment:
Yes. When customers roll out a new versions for *participants*, they should expect the behavior to change accordingly. This system property is for an emergency rollback, during which users can disable this new behavior.
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -84,4 +84,7 @@
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
public static final String STATEUPDATEUTIL_ERROR_PERSISTENCY_ENABLED = "helix.StateUpdateUtil.errorLog.enabled";
+
+ public static final String TASK_CURRENT_STATE_PATH_DISABLED =
Review comment:
Yes. When customers roll out a new versions for *participants*, they should expect the behavior to change accordingly. This system property is for an emergency controller rollback, during which users can disable this new behavior on participants.
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r541262471
##########
File path: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
##########
@@ -264,8 +271,12 @@ void postHandleMessage() {
try {
// Update the ZK current state of the node
- PropertyKey key = keyBuilder.currentState(instanceName, sessionId, resource,
- bucketizer.getBucketName(partitionKey));
+ PropertyKey key =
+ _isTaskMessage && !Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED)
Review comment:
Ah... Yeah, the "disable" should be false.
But for dynamic update, let's not overkill the design:
1. For Helix, we use ZK centralized config for dynamic update purpose. If we decide to use system property, it means it is not dynamic changeable.
2. The system property mostly comes from Java property, which require restart the application. So we cannot support dynamic update as well.
Make it a variable can also simplify the code. I would suggest create a variable to hold that when we create the object.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r541121512
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskRunner.java
##########
@@ -208,7 +209,10 @@ private static boolean setRequestedState(HelixDataAccessor accessor, String inst
String.format("Requesting a state transition to %s for partition %s.", state, partition));
try {
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
- PropertyKey key = keyBuilder.currentState(instance, sessionId, resource);
+ PropertyKey key =
+ Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED) ? keyBuilder
Review comment:
For this I believe there's a similar problem: if users change this flag on the fly, the path has to be recreated. If we create the path when TaskRunner is instantiated, there's no way to recreate the path.
----------------------------------------------------------------
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
[GitHub] [helix] alirezazamani merged pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
alirezazamani merged pull request #1584:
URL: https://github.com/apache/helix/pull/1584
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r540594862
##########
File path: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
##########
@@ -264,8 +271,12 @@ void postHandleMessage() {
try {
// Update the ZK current state of the node
- PropertyKey key = keyBuilder.currentState(instanceName, sessionId, resource,
- bucketizer.getBucketName(partitionKey));
+ PropertyKey key =
+ _isTaskMessage && !Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED)
Review comment:
Default value should be false because it's "disabled" right? If participants get the new version of Helix, we should assume that they want to use the new path, so "disabled" should default to false.
For variable tracking, there is a problem: if the boolean value is recorded at a central place, users cannot update this system property during run time, right? If there is a variable that's tracking it, then it's as if it's cached.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on pull request #1584:
URL: https://github.com/apache/helix/pull/1584#issuecomment-745606714
This PR is ready to be merged, approved by @dasahcc @alirezazamani
Final commit message:
## Participant-side Task Current State Migration ##
Second part of task current state migration. All changes made in this PR are on the participant side.
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r540560573
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskRunner.java
##########
@@ -208,7 +209,10 @@ private static boolean setRequestedState(HelixDataAccessor accessor, String inst
String.format("Requesting a state transition to %s for partition %s.", state, partition));
try {
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
- PropertyKey key = keyBuilder.currentState(instance, sessionId, resource);
+ PropertyKey key =
+ Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED) ? keyBuilder
Review comment:
Let's have the path build when TaskRunner instantiated.
##########
File path: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
##########
@@ -264,8 +271,12 @@ void postHandleMessage() {
try {
// Update the ZK current state of the node
- PropertyKey key = keyBuilder.currentState(instanceName, sessionId, resource,
- bucketizer.getBucketName(partitionKey));
+ PropertyKey key =
+ _isTaskMessage && !Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED)
Review comment:
Let's have a variable to track TASK_CURRENT_STATE_PATH_DISABLED. We dont have to do the parse every time. Also, let's give a default value as true since participant will be the last component to deploy.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r542931895
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -84,4 +84,7 @@
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
public static final String STATEUPDATEUTIL_ERROR_PERSISTENCY_ENABLED = "helix.StateUpdateUtil.errorLog.enabled";
+
+ public static final String TASK_CURRENT_STATE_PATH_DISABLED =
Review comment:
Yes. When customers roll out a new versions for *participants*, they should expect the behavior to change accordingly. This system property is for an emergency rollback.
----------------------------------------------------------------
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
[GitHub] [helix] dasahcc commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r541262817
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskRunner.java
##########
@@ -208,7 +209,10 @@ private static boolean setRequestedState(HelixDataAccessor accessor, String inst
String.format("Requesting a state transition to %s for partition %s.", state, partition));
try {
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
- PropertyKey key = keyBuilder.currentState(instance, sessionId, resource);
+ PropertyKey key =
+ Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED) ? keyBuilder
Review comment:
Same reason as the previous, dynamic update is overkill.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r542831848
##########
File path: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
##########
@@ -264,8 +271,12 @@ void postHandleMessage() {
try {
// Update the ZK current state of the node
- PropertyKey key = keyBuilder.currentState(instanceName, sessionId, resource,
- bucketizer.getBucketName(partitionKey));
+ PropertyKey key =
+ _isTaskMessage && !Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED)
Review comment:
Concluded to use one local variable when necessary.
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskRunner.java
##########
@@ -208,7 +209,10 @@ private static boolean setRequestedState(HelixDataAccessor accessor, String inst
String.format("Requesting a state transition to %s for partition %s.", state, partition));
try {
PropertyKey.Builder keyBuilder = accessor.keyBuilder();
- PropertyKey key = keyBuilder.currentState(instance, sessionId, resource);
+ PropertyKey key =
+ Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED) ? keyBuilder
Review comment:
Concluded to use one local variable when necessary.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r542579208
##########
File path: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
##########
@@ -264,8 +271,12 @@ void postHandleMessage() {
try {
// Update the ZK current state of the node
- PropertyKey key = keyBuilder.currentState(instanceName, sessionId, resource,
- bucketizer.getBucketName(partitionKey));
+ PropertyKey key =
+ _isTaskMessage && !Boolean.getBoolean(SystemPropertyKeys.TASK_CURRENT_STATE_PATH_DISABLED)
Review comment:
Discussion ongoing.
----------------------------------------------------------------
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
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1584: Participant-side Task Current State Migration
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1584:
URL: https://github.com/apache/helix/pull/1584#discussion_r542919905
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -84,4 +84,7 @@
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
public static final String STATEUPDATEUTIL_ERROR_PERSISTENCY_ENABLED = "helix.StateUpdateUtil.errorLog.enabled";
+
+ public static final String TASK_CURRENT_STATE_PATH_DISABLED =
Review comment:
With this config set up as "disabled", the default behavior would be to turn on task current state. Is this the expected behavior when customers roll out?
----------------------------------------------------------------
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