You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/23 17:18:06 UTC
[GitHub] [beam] Abacn opened a new pull request, #22023: Fix SpannerIO runBatchQueryTestWithFailures flakes
Abacn opened a new pull request, #22023:
URL: https://github.com/apache/beam/pull/22023
This fixes #21999
* Fix runBatchQueryTestWithFailures api call verify condition.
Also remove PAssert that never executes due to exception thrown upstream
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165950540
JavaPreCommit failed due to StyleCheck violation in master which is just fixed in #22046
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165592371
oops #21729 and #22008 are duplicated and I did not notice the later by the time of this solution.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
nielm commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165658575
> This approach works too, though I'd go the extra step and "invert" the test---method per error type and then testing the responses from single and batched writes via a helper method rather than special casing the error type in the loop
I was also half-way through a PR for this.
In my solution I was just going to switch from numSleeps, numWrites parameters to a "hasRetries" parameter, then then if there are retries assert that numSleeps >= 8 and numWrites >= 9
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #22023: Fix SpannerIO runBatchQueryTestWithFailures flakes
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1164672104
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem merged pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
pabloem merged PR #22023:
URL: https://github.com/apache/beam/pull/22023
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #22023: Fix SpannerIO runBatchQueryTestWithFailures flakes
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1164672103
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on a diff in pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
nielm commented on code in PR #22023:
URL: https://github.com/apache/beam/pull/22023#discussion_r906150891
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIOWriteExceptionHandlingTest.java:
##########
@@ -73,49 +70,43 @@ public static Collection<Object[]> data() {
return Arrays.asList(
new Object[][] {
// DEADLINE_EXCEEDED is the only exception type that generates retries in-SDK
- // The default backoff generates 9 retries, and then errors out the pipeline.
- {ErrorCode.DEADLINE_EXCEEDED, "deadline passed!", 9, 10},
+ // The default backoff generates _ABOUT_ 9 retries, and then errors out the pipeline.
+ {ErrorCode.DEADLINE_EXCEEDED, "deadline passed!"},
Review Comment:
Rather than testing for DEADLINE_EXEEDED in the assert, put a hasRetries parameter here, and use that to determine whether to test no sleeps and 1 write, or writes >=9 and sleeps => 8
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] byronellis commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
byronellis commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165600870
Yes, same one
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
pabloem commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1166068494
ok lgtm
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165949889
R: @pabloem
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] byronellis commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
byronellis commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165608856
This approach works too, though I'd go the extra step and "invert" the test---method per error type and then testing the responses from single and batched writes via a helper method rather than special casing the error type in the loop
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] byronellis commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
byronellis commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165611710
[Another approach](https://github.com/apache/beam/pull/22035) over 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on a diff in pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
nielm commented on code in PR #22023:
URL: https://github.com/apache/beam/pull/22023#discussion_r906145815
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIOWriteExceptionHandlingTest.java:
##########
@@ -171,9 +163,21 @@ public void testExceptionHandlingForSimpleWrite() throws InterruptedException {
try {
pipeline.run().waitUntilFinish();
} finally {
- verify(SpannerIO.WriteToSpannerFn.sleeper, times(callsToSleeper)).sleep(anyLong());
- verify(serviceFactory.mockDatabaseClient(), times(callsToWrite))
- .writeAtLeastOnceWithOptions(any(), any(Options.ReadQueryUpdateTransactionOption.class));
+ if (exceptionErrorcode == ErrorCode.DEADLINE_EXCEEDED) {
+ // Due to randomOffset retry is done by most likely but not always 9 times.
+ int sleepCalled =
+ Mockito.mockingDetails(SpannerIO.WriteToSpannerFn.sleeper).getInvocations().size();
+ assertTrue(sleepCalled >= 8);
+ assertTrue(sleepCalled <= 10);
Review Comment:
That was more effort than I was prepared to do - I was just going to test that steeps >= 8 and writes >= 9!
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165626219
> This approach works too, though I'd go the extra step and "invert" the test---method per error type and then testing the responses from single and batched writes via a helper method rather than special casing the error type in the loop
Time_exceed and other error types are treated differently in the tested DoFn and that's why I did branching here. Fine to me since they fixes flakes.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #22023: Fix SpannerIO runBatchQueryTestWithFailures flakes
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1164672099
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] byronellis commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
byronellis commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165662455
It's more a comment on the structure of the test suite. In the original suite, which assumes all the results are deterministic, you want to test many different error scenarios parameterized by number of retries (number of calls parameter is redundant as its always number of retries + 1)
Fixing DEADLINE_EXCEEDED changes by putting the number of retries into a range. That suggests a third maybe even cleaner approach which would be to specify a min and max number of expected calls and then just drop the sleeper check entirely.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on code in PR #22023:
URL: https://github.com/apache/beam/pull/22023#discussion_r905553948
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIOWriteExceptionHandlingTest.java:
##########
@@ -171,9 +163,21 @@ public void testExceptionHandlingForSimpleWrite() throws InterruptedException {
try {
pipeline.run().waitUntilFinish();
} finally {
- verify(SpannerIO.WriteToSpannerFn.sleeper, times(callsToSleeper)).sleep(anyLong());
- verify(serviceFactory.mockDatabaseClient(), times(callsToWrite))
- .writeAtLeastOnceWithOptions(any(), any(Options.ReadQueryUpdateTransactionOption.class));
+ if (exceptionErrorcode == ErrorCode.DEADLINE_EXCEEDED) {
+ // Due to randomOffset retry is done by most likely but not always 9 times.
+ int sleepCalled =
+ Mockito.mockingDetails(SpannerIO.WriteToSpannerFn.sleeper).getInvocations().size();
+ assertTrue(sleepCalled >= 8);
+ assertTrue(sleepCalled <= 10);
Review Comment:
Ran backOff 1 milliion times and found number of retry is distributed as: {8=52661, 9=878266, 10=69073}. i.e. Originally it has a flake probability of 12%
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1164974523
R: @nielm
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #22023: Fix SpannerIO runBatchQueryTestWithFailures flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1164739898
R: @johnjcasey
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
Abacn commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165661811
>
Yeah agree. Exact number retry is implementation details of backOff which is not needed to test here. Dropped this approach as this test is fixed in #22035.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #22023: Fix SpannerIO runBatchQueryTestWithFailures flakes
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1164672095
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #22023: Fix SpannerIO runBatchQueryTestWithFailures flakes
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1164672102
Can one of the admins verify this patch?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] johnjcasey commented on pull request #22023: Fix SpannerIO flakes
Posted by GitBox <gi...@apache.org>.
johnjcasey commented on PR #22023:
URL: https://github.com/apache/beam/pull/22023#issuecomment-1165541633
@byronellis is this the same issue you were looking at?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org