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/06/18 00:35:14 UTC
[GitHub] [helix] kaisun2000 opened a new pull request #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
kaisun2000 opened a new pull request #1104:
URL: https://github.com/apache/helix/pull/1104
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1102
TestRemoveUserCbHandlerOnPathRemoval has a flaky test
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
`Assert
.assertEquals(watchPaths.get("existWatches").size(), 2,
"Should have 2 exist-watches: CURRENTSTATE/{oldSessionId} and CURRENTSTATE/{oldSessionId}/TestDB0");`
is flaky.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
Waiting result for now.
### Commits
- [x] 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"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [x] 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] kaisun2000 commented on a change in pull request #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1104:
URL: https://github.com/apache/helix/pull/1104#discussion_r442567666
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestZkCallbackHandlerLeak.java
##########
@@ -605,9 +605,12 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
"Should have 1 data-watches: MESSAGES");
Assert.assertEquals(watchPaths.get("childWatches").size(), 1,
"Should have 1 child-watches: MESSAGES");
- Assert
- .assertEquals(watchPaths.get("existWatches").size(), 2,
- "Should have 2 exist-watches: CURRENTSTATE/{oldSessionId} and CURRENTSTATE/{oldSessionId}/TestDB0");
+ result = TestHelper.verify(()-> {
+ Map<String, List<String>> wPaths = ZkTestHelper.getZkWatch(participantToExpire.getZkClient());
+ return wPaths.get("existWatches").size() == 1;
+ }, 10000);
+ Assert.assertTrue(result,
+ "Should have 1 exist-watches: CURRENTSTATE/{oldSessionId}");
Review comment:
This test is confusing. Note here it is a participant0, register and currentstate/{sessionid} callbackhandler of itself.
This does not normally happen in production,
The only case this can happen is that controller have CB for participant 1. At the same time controller and participant 1 both have session expiration.
Back to the question, yes, the old session will be removed from participant point of view. But from router (who subscribe to callbackhandler), each existing callbackhandler of router would be called via lambda from zkclient event queue inserted by fireAllEvent().
` fireChildChangedEvents(entry.getKey(), entry.getValue(), true);`
The lambda would install this watch.
Note, this is normally not a problem as we don't have such usage case. Only if both participant and controller/router have session expiration at the same time.
Later, we plan to make it clear the responsibility of who should install watcher. We can simplify the logic here.
----------------------------------------------------------------
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 #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1104:
URL: https://github.com/apache/helix/pull/1104#discussion_r442567666
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestZkCallbackHandlerLeak.java
##########
@@ -605,9 +605,12 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
"Should have 1 data-watches: MESSAGES");
Assert.assertEquals(watchPaths.get("childWatches").size(), 1,
"Should have 1 child-watches: MESSAGES");
- Assert
- .assertEquals(watchPaths.get("existWatches").size(), 2,
- "Should have 2 exist-watches: CURRENTSTATE/{oldSessionId} and CURRENTSTATE/{oldSessionId}/TestDB0");
+ result = TestHelper.verify(()-> {
+ Map<String, List<String>> wPaths = ZkTestHelper.getZkWatch(participantToExpire.getZkClient());
+ return wPaths.get("existWatches").size() == 1;
+ }, 10000);
+ Assert.assertTrue(result,
+ "Should have 1 exist-watches: CURRENTSTATE/{oldSessionId}");
Review comment:
This test is confusing. Note here it is a participant0, register and currentstate/{sessionid} callbackhandler of itself.
This does not normally happen in production,
The only case this can happen is that controller have CB for participant 1. At the same time controller and participant 1 both have session expiration.
Back to the question, yes, the old session will be removed from participant point of view. But from router point of view (participant0 is also router as it subscribe to callbackhandler of current state), each existing callbackhandler of router would be called via lambda from zkclient event queue inserted by fireAllEvent() after session expiration.
` fireChildChangedEvents(entry.getKey(), entry.getValue(), true);`
The lambda would install this watch.
Note, this is normally not a problem as we don't have such usage case. Only if both participant and controller/router have session expiration at the same time.
Later, we plan to make it clear the responsibility of who should install watcher. We can simplify the logic here.
----------------------------------------------------------------
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 pull request #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1104:
URL: https://github.com/apache/helix/pull/1104#issuecomment-646893555
This PR is ready to be merged, approved by @jiajunwang
> Fix flaky test testRemoveUserCbHandlerOnPathRemoval # #1102
>
> TestRemoveUserCbHandlerOnPathRemoval has a flaky assert. Addres
> the assertion in this diff.
----------------------------------------------------------------
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 #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1104:
URL: https://github.com/apache/helix/pull/1104#discussion_r443065588
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestZkCallbackHandlerLeak.java
##########
@@ -605,9 +605,12 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
"Should have 1 data-watches: MESSAGES");
Assert.assertEquals(watchPaths.get("childWatches").size(), 1,
"Should have 1 child-watches: MESSAGES");
- Assert
- .assertEquals(watchPaths.get("existWatches").size(), 2,
- "Should have 2 exist-watches: CURRENTSTATE/{oldSessionId} and CURRENTSTATE/{oldSessionId}/TestDB0");
+ result = TestHelper.verify(()-> {
+ Map<String, List<String>> wPaths = ZkTestHelper.getZkWatch(participantToExpire.getZkClient());
+ return wPaths.get("existWatches").size() == 1;
+ }, 10000);
+ Assert.assertTrue(result,
+ "Should have 1 exist-watches: CURRENTSTATE/{oldSessionId}");
Review comment:
Good point, added the comment.
----------------------------------------------------------------
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] jiajunwang merged pull request #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1104:
URL: https://github.com/apache/helix/pull/1104
----------------------------------------------------------------
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] jiajunwang commented on a change in pull request #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1104:
URL: https://github.com/apache/helix/pull/1104#discussion_r442584471
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestZkCallbackHandlerLeak.java
##########
@@ -605,9 +605,12 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
"Should have 1 data-watches: MESSAGES");
Assert.assertEquals(watchPaths.get("childWatches").size(), 1,
"Should have 1 child-watches: MESSAGES");
- Assert
- .assertEquals(watchPaths.get("existWatches").size(), 2,
- "Should have 2 exist-watches: CURRENTSTATE/{oldSessionId} and CURRENTSTATE/{oldSessionId}/TestDB0");
+ result = TestHelper.verify(()-> {
+ Map<String, List<String>> wPaths = ZkTestHelper.getZkWatch(participantToExpire.getZkClient());
+ return wPaths.get("existWatches").size() == 1;
+ }, 10000);
+ Assert.assertTrue(result,
+ "Should have 1 exist-watches: CURRENTSTATE/{oldSessionId}");
Review comment:
Make sense. Thanks for the explanation. Could you please add the critical points of the explanation to the comment of the code? I think we will need to improve this further, and for sure the test case needs to be updated accordingly. Then with this additional info, it would easier to understand what shall we expect in this scenario later.
----------------------------------------------------------------
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] jiajunwang commented on a change in pull request #1104: Enhance flaky test TestRemoveUserCbHandlerOnPathRemoval
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1104:
URL: https://github.com/apache/helix/pull/1104#discussion_r442525807
##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestZkCallbackHandlerLeak.java
##########
@@ -605,9 +605,12 @@ public void onStateChange(String instanceName, List<CurrentState> statesInfo,
"Should have 1 data-watches: MESSAGES");
Assert.assertEquals(watchPaths.get("childWatches").size(), 1,
"Should have 1 child-watches: MESSAGES");
- Assert
- .assertEquals(watchPaths.get("existWatches").size(), 2,
- "Should have 2 exist-watches: CURRENTSTATE/{oldSessionId} and CURRENTSTATE/{oldSessionId}/TestDB0");
+ result = TestHelper.verify(()-> {
+ Map<String, List<String>> wPaths = ZkTestHelper.getZkWatch(participantToExpire.getZkClient());
+ return wPaths.get("existWatches").size() == 1;
+ }, 10000);
+ Assert.assertTrue(result,
+ "Should have 1 exist-watches: CURRENTSTATE/{oldSessionId}");
Review comment:
Please correct me if I am wrong.
When the participant handles the new session, this folder of the old session will be removed, right? With the new change, should the watcher be removed as well when the folder being deleted?
----------------------------------------------------------------
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