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/07/29 07:44:09 UTC
[GitHub] [helix] alirezazamani opened a new pull request #1180: Stabilize TestTaskSchedulingTwoCurrentStates
alirezazamani opened a new pull request #1180:
URL: https://github.com/apache/helix/pull/1180
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR title:
Fixes #1179
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
In this PR, TestTaskSchedulingTwoCurrentStates has been stabilized by creating SEMI-AUTO resource and update the preferenceList to avoid race condition that happens when test and controller update the preferenceList at the same time.
### Tests
- [x] The following is the result of the "mvn test" command on the appropriate module:
helix-core:
```
[INFO] Tests run: 1155, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4,800.964 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1155, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:20 h
[INFO] Finished at: 2020-07-27T14:59:27-07:00
[INFO] ------------------------------------------------------------------------
```
helix-rest:
```
[INFO] Tests run: 163, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 43.032 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 163, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 48.283 s
[INFO] Finished at: 2020-07-27T15:07:54-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] dasahcc merged pull request #1180: Remove race condition in TestTaskSchedulingTwoCurrentStates
Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #1180:
URL: https://github.com/apache/helix/pull/1180
----------------------------------------------------------------
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 #1180: Remove race condition in TestTaskSchedulingTwoCurrentStates
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1180:
URL: https://github.com/apache/helix/pull/1180#discussion_r461799894
##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskSchedulingTwoCurrentStates.java
##########
@@ -65,7 +60,7 @@
* sent when there are two CurrentStates.
*/
public class TestTaskSchedulingTwoCurrentStates extends TaskTestBase {
- private static final String DATABASE = WorkflowGenerator.DEFAULT_TGT_DB;
Review comment:
The main reason is that in before class method, "TestDB" is created as Full-Auto and in this test we are trying to change it to Semi-Auto which causes race condition between controller and test updating IdealState. So if want to have Semi-Auto resource, we need to create it separately (with different name) and leverage it in this test.
##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskSchedulingTwoCurrentStates.java
##########
@@ -65,7 +60,7 @@
* sent when there are two CurrentStates.
*/
public class TestTaskSchedulingTwoCurrentStates extends TaskTestBase {
- private static final String DATABASE = WorkflowGenerator.DEFAULT_TGT_DB;
Review comment:
Your point is somehow valid. But I still think this is better option because this way we are saving many duplicated code which will be hard to maintain and most of the code is written in the parent class. Instead of that, as what I proposed in this PR, we can separate the DATABASE name and create new one which includes this class name and will be unique. We have same behavior for most of the Task Framework related test and workflow names have been derived from class name. This way the tests is easier to maintain and more understandable.
##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskSchedulingTwoCurrentStates.java
##########
@@ -65,7 +60,7 @@
* sent when there are two CurrentStates.
*/
public class TestTaskSchedulingTwoCurrentStates extends TaskTestBase {
- private static final String DATABASE = WorkflowGenerator.DEFAULT_TGT_DB;
+ private static final String DATABASE = "TestDB_" + TestHelper.getTestClassName();
Review comment:
Ok I added afterclass which basically calling super.afterclass and we are cleaning up the environment. Thanks for the feedback.
----------------------------------------------------------------
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] kaisun2000 commented on a change in pull request #1180: Remove race condition in TestTaskSchedulingTwoCurrentStates
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1180:
URL: https://github.com/apache/helix/pull/1180#discussion_r461796364
##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskSchedulingTwoCurrentStates.java
##########
@@ -65,7 +60,7 @@
* sent when there are two CurrentStates.
*/
public class TestTaskSchedulingTwoCurrentStates extends TaskTestBase {
- private static final String DATABASE = WorkflowGenerator.DEFAULT_TGT_DB;
Review comment:
Is it possible the old way of using WorkflowGenerator.DEFAULT_TGT_DB would have conflict, if multiple test using the same "WorkflowGenerator.DEFAULT_TGT_DB"?
##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskSchedulingTwoCurrentStates.java
##########
@@ -65,7 +60,7 @@
* sent when there are two CurrentStates.
*/
public class TestTaskSchedulingTwoCurrentStates extends TaskTestBase {
- private static final String DATABASE = WorkflowGenerator.DEFAULT_TGT_DB;
Review comment:
I think the better way to fix it is not call super.beforeClass() in line 72/77. This way, we won't create the cluster/full auto etc and participant that we need to stop immediately.
Instead, just creates cluster, controller and participant in this before test. What is your take?
##########
File path: helix-core/src/test/java/org/apache/helix/integration/task/TestTaskSchedulingTwoCurrentStates.java
##########
@@ -65,7 +60,7 @@
* sent when there are two CurrentStates.
*/
public class TestTaskSchedulingTwoCurrentStates extends TaskTestBase {
- private static final String DATABASE = WorkflowGenerator.DEFAULT_TGT_DB;
+ private static final String DATABASE = "TestDB_" + TestHelper.getTestClassName();
Review comment:
Another thing is that for all the resources created in
```
@BeforeClass
public void beforeClass() throws Exception {
```
We should tear them down. The participant threads, the controller thread should be stopped with syncStop. Maybe we should also clean up the cluster path etc in Zookeeper.
----------------------------------------------------------------
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 #1180: Remove race condition in TestTaskSchedulingTwoCurrentStates
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on pull request #1180:
URL: https://github.com/apache/helix/pull/1180#issuecomment-665772090
This PR is ready to be merged, approved by @dasahcc
Final commit message:
Remove race condition in TestTaskSchedulingTwoCurrentStates
In this commit, TestTaskSchedulingTwoCurrentStates has been stabilized
by creating SEMI-AUTO resource and update the preferenceList to avoid
race condition that happens when test and controller update the
preferenceList at the same time.
----------------------------------------------------------------
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