You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "desaikomal (via GitHub)" <gi...@apache.org> on 2023/05/24 06:40:42 UTC
[GitHub] [helix] desaikomal opened a new pull request, #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
desaikomal opened a new pull request, #2504:
URL: https://github.com/apache/helix/pull/2504
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #2503
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
The test waits for the update and what is the cache. At the end of the day, callbacks are asynchronous and sometimes they don't get the same count.
Increased the attempts from 10 to 20. The test doesn't wait by default. Instead if results matches, it will break quickly. Only when things are not converging, it will iterate more.
### 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:
I ran the same test through different branch, thanks @xyuanlu for showing how to run the test.
https://github.com/desaikomal/helix/pull/1 - i have run successfully couple of times (not exactly 20 times).
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### 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
[GitHub] [helix] desaikomal commented on pull request #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on PR #2504:
URL: https://github.com/apache/helix/pull/2504#issuecomment-1565581144
Thanks @Marcosrico + @parakhnr for reviewing the change. This change has been approved by @Marcosrico and @parakhnr and ready to be merged.
Commit message: Fix TestZkCacheAsyncOpSingleThread test failure.
--
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
[GitHub] [helix] desaikomal commented on a diff in pull request #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2504:
URL: https://github.com/apache/helix/pull/2504#discussion_r1206817238
##########
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkCacheAsyncOpSingleThread.java:
##########
@@ -174,15 +174,14 @@ public void testHappyPathExtOpZkCacheBaseDataAccessor() throws Exception {
// wait zkEventThread update zkCache
// verify wtCache
- for (int i = 0; i < 10; i++) {
+ for (int i = 0; i < 20; i++) {
Review Comment:
@parakhnr - as we discussed in other PR, the other verifier has a timeout of 30sec within itself and so having nested verifier doesn't make sense.
Can you please review it again in the light of above?
--
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
[GitHub] [helix] rahulrane50 commented on a diff in pull request #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
Posted by "rahulrane50 (via GitHub)" <gi...@apache.org>.
rahulrane50 commented on code in PR #2504:
URL: https://github.com/apache/helix/pull/2504#discussion_r1204631874
##########
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkCacheAsyncOpSingleThread.java:
##########
@@ -174,15 +174,14 @@ public void testHappyPathExtOpZkCacheBaseDataAccessor() throws Exception {
// wait zkEventThread update zkCache
// verify wtCache
- for (int i = 0; i < 10; i++) {
+ for (int i = 0; i < 20; i++) {
Review Comment:
We can also run this test 20 times and see if t really fixes the flaky test.
--
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
[GitHub] [helix] xyuanlu merged pull request #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2504:
URL: https://github.com/apache/helix/pull/2504
--
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
[GitHub] [helix] parakhnr commented on a diff in pull request #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
Posted by "parakhnr (via GitHub)" <gi...@apache.org>.
parakhnr commented on code in PR #2504:
URL: https://github.com/apache/helix/pull/2504#discussion_r1207398730
##########
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkCacheAsyncOpSingleThread.java:
##########
@@ -174,15 +174,14 @@ public void testHappyPathExtOpZkCacheBaseDataAccessor() throws Exception {
// wait zkEventThread update zkCache
// verify wtCache
- for (int i = 0; i < 10; i++) {
+ for (int i = 0; i < 20; i++) {
Review Comment:
Hmmm...we were verifying the cluster state in the previous one which had 30 seconds timeout. For `verifyZkCache` We don't have timeouts or sleeps involved, so I think we should be able to use `TestHelper.verify` over here with a timeout of `20 * (100 for sleep + 50 executing of the verifyZkCache) millis` to begin with.
--
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
[GitHub] [helix] desaikomal commented on a diff in pull request #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2504:
URL: https://github.com/apache/helix/pull/2504#discussion_r1204668225
##########
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkCacheAsyncOpSingleThread.java:
##########
@@ -174,15 +174,14 @@ public void testHappyPathExtOpZkCacheBaseDataAccessor() throws Exception {
// wait zkEventThread update zkCache
// verify wtCache
- for (int i = 0; i < 10; i++) {
+ for (int i = 0; i < 20; i++) {
Review Comment:
you can see here: https://github.com/desaikomal/helix/pull/1 - i have run successfully couple of times (not exactly 20 times).
--
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
[GitHub] [helix] parakhnr commented on a diff in pull request #2504: Fix test failure: TestZkCacheAsyncOpSingleThread
Posted by "parakhnr (via GitHub)" <gi...@apache.org>.
parakhnr commented on code in PR #2504:
URL: https://github.com/apache/helix/pull/2504#discussion_r1204576523
##########
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkCacheAsyncOpSingleThread.java:
##########
@@ -174,15 +174,14 @@ public void testHappyPathExtOpZkCacheBaseDataAccessor() throws Exception {
// wait zkEventThread update zkCache
// verify wtCache
- for (int i = 0; i < 10; i++) {
+ for (int i = 0; i < 20; i++) {
Review Comment:
Let's use `TestHelper.verify` with appropriate timeout just to stay consistent.
--
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