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/08/05 22:25:47 UTC
[GitHub] [helix] alirezazamani opened a new pull request #1221: Drop the task related current states for new session
alirezazamani opened a new pull request #1221:
URL: https://github.com/apache/helix/pull/1221
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR title:
Fixes #1220
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
Since the controller is not calculating quota based on current state, there is no need to set requested state to DROPPED and ask controller to drop the current state. Instead, in this commit, the participant can drop the current state right away.
### Tests
- [x] The following tests are written for this issue:
TestTaskCurrentStateDrop
- [x] The following is the result of the "mvn test" command on the appropriate module:
helix-core:
```
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestEnableCompression.testEnableCompressionResource:117 expected:<true> but was:<false>
[ERROR] TestClusterStateVerifier.afterMethod:98 » IllegalState ZkClient already closed...
[ERROR] TestClusterStateVerifier.testResourceSubset:130 » IllegalState ZkClient alread...
[INFO]
[ERROR] Tests run: 1167, Failures: 3, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:22 h
[INFO] Finished at: 2020-08-05T14:41:04-07:00
[INFO] ------------------------------------------------------------------------
```
The failed test passed when run individually.
mvn test -Dtest="TestClusterStateVerifier"
```
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.327 s - in org.apache.helix.tools.TestClusterStateVerifier
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 15.758 s
[INFO] Finished at: 2020-08-05T15:19:08-07:00
[INFO] ------------------------------------------------------------------------
```
mvn test -Dtest="TestEnableCompression"
```
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 30.005 s - in org.apache.helix.integration.TestEnableCompression
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 36.413 s
[INFO] Finished at: 2020-08-05T15:22:55-07:00
[INFO] ------------------------------------------------------------------------
```
helix-rest:
```
[INFO] Tests run: 164, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 41.469 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 164, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 47.189 s
[INFO] Finished at: 2020-08-05T15:24:23-07:00
[INFO] ------------------------------------------------------------------------
```
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. 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
- [x] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
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 commented on pull request #1221: Drop the task related current states for new session
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on pull request #1221:
URL: https://github.com/apache/helix/pull/1221#issuecomment-671580998
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.
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 commented on a change in pull request #1221: (WIP) Drop the task related current states for new session
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1221:
URL: https://github.com/apache/helix/pull/1221#discussion_r467270900
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -350,6 +351,12 @@ private void carryOverPreviousCurrentState() {
+ lastCurState);
continue;
}
+
+ // If the the current state is related to tasks, there is no need to carry it over to new session.
+ if (stateModelDefRef.equals(TaskConstants.STATE_MODEL_NAME)) {
+ continue;
+ }
Review comment:
When we establishing new session, we will carryover the old current states. If we skip this carry over for the task related current states, then the session will no longer have task related current states. Hence, there is no need to the controller to drop the current state in this case.
----------------------------------------------------------------
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 commented on pull request #1221: (WIP) Drop the task related current states for new session
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on pull request #1221:
URL: https://github.com/apache/helix/pull/1221#issuecomment-670716214
> Another thing, if we directly drop current state, shall we have some mechanism in the participant side to cancel all running tasks before we reset all the thread pool? If there are some data need to be cleared.
I just checked the HelixStateMachineEngine file. When we call reset() in this file, for the tasks we call _taskRunner.cancel() which cancels the tasks on the participant. So I think if we skip the currentState carry over operation for the tasks, we are good to go. Please let me know if that answers your questions. Thanks.
----------------------------------------------------------------
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 #1221: Drop the task related current states for new session
Posted by GitBox <gi...@apache.org>.
alirezazamani merged pull request #1221:
URL: https://github.com/apache/helix/pull/1221
----------------------------------------------------------------
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 #1221: Drop the task related current states for new session
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1221:
URL: https://github.com/apache/helix/pull/1221#discussion_r467216882
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -350,6 +351,12 @@ private void carryOverPreviousCurrentState() {
+ lastCurState);
continue;
}
+
+ // If the the current state is related to tasks, there is no need to carry it over to new session.
+ if (stateModelDefRef.equals(TaskConstants.STATE_MODEL_NAME)) {
+ continue;
+ }
Review comment:
If we skip it, do we skip the operation for deleting old current states?
----------------------------------------------------------------
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