Suppress BigQuery read stream splitAtFraction when API busy call or timeout #31125

Abacn · 2024-04-27T00:54:11Z

Resolve #30646 (comment)

Reject split attempt early when there is an ongoing hasNext call in work item thread
TImeout hasNext call in splitAtFraction in alignment with client splitReadStreamSettings

The issue exist on both released Beam and after #31096

Tested on read stream API v1 (default) and Dataflow legacy runner in a quota restricted project:

Without the change, the pipeline appears to stuck indefinitely. the onRetryAttempt callback get called periodically.

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:

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.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

…imeout * Reject split attempt early when there is an ongoing hasNext call in work item thread * TImeout hasNext call in splitAtFraction in alignment with client splitReadStreamSettings

Abacn · 2024-04-27T00:59:35Z

...-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryStorageStreamSource.java

+          try {
+            // The intended wait time is in sync with splitReadStreamSettings.setRetrySettings in
+            // StorageClientImpl.
+            future.get(30, TimeUnit.SECONDS);


This is correspond to the config here

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryServicesImpl.java

Lines 1735 to 1742 in 673da54

splitReadStreamSettings.setRetrySettings(

splitReadStreamSettings

.getRetrySettings()

.toBuilder()

.setInitialRpcTimeout(org.threeten.bp.Duration.ofSeconds(30))

.setMaxRpcTimeout(org.threeten.bp.Duration.ofSeconds(30))

.setTotalTimeout(org.threeten.bp.Duration.ofSeconds(30))

.build());

It intended to set the API call timeout during split request to 30s (and otherwise reject split). However, a bug fix #21739 added this call here, which has internal retry logic, but this added call is not covered by the split-stream-retry config above. When there is quota issue, this is causing pipeline stuck

Abacn · 2024-04-27T01:00:58Z

...ud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOStorageReadTest.java

+    latch.await();
+
+    // Beam does not split storage read api v2 stream
+    assertNull(reader.splitAtFraction(0.5));


without the changes in main scope code, the test fails with

java.lang.NullPointerException at org.apache.beam.sdk.io.gcp.bigquery.BigQueryStorageStreamSource$BigQueryStorageStreamReader.splitAtFraction(BigQueryStorageStreamSource.java:372)

because it passed "splitAllowed" check

github-actions · 2024-04-27T01:51:32Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Abacn · 2024-04-27T03:30:34Z

...-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryStorageStreamSource.java

@@ -388,8 +398,22 @@ public synchronized BigQueryStorageStreamSource<T> getCurrentSource() {
          // The following line is required to trigger the `FailedPreconditionException` on which
          // the SplitReadStream validation logic depends. Removing it will cause incorrect
          // split operations to succeed.
-          newResponseIterator.hasNext();
-          storageClient.reportPendingMetrics();


removed reportPendingMetrics call here otherwise same warning as #31096 seen, because splitAtFraction call thread isn't a work item thread

github-actions · 2024-04-29T18:06:10Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damondouglas for label java.
R: @chamikaramj for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Abacn · 2024-04-30T20:00:40Z

R: @ahmedabu98 (since you reviewed #31096)

github-actions · 2024-04-30T20:02:10Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

ahmedabu98

LGTM, just some cleanup

...ud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOStorageReadTest.java

...-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryStorageStreamSource.java

codecov-commenter · 2024-05-06T19:27:50Z

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 60.41%. Comparing base (b34cf54) to head (4b3dd6a).
Report is 122 commits behind head on master.

Files	Patch %	Lines
...k/io/gcp/bigquery/BigQueryStorageStreamSource.java	53.84%	6 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #31125       +/-   ##
=============================================
- Coverage     71.44%   60.41%   -11.04%     
- Complexity     1474     2981     +1507     
=============================================
  Files           906      659      -247     
  Lines        113271    66095    -47176     
  Branches       1076     3233     +2157     
=============================================
- Hits          80931    39930    -41001     
+ Misses        30327    23066     -7261     
- Partials       2013     3099     +1086

Flag	Coverage Δ
java	`68.62% <53.84%> (+0.04%)`	⬆️
python	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Suppress BigQuery read stream splitAtFraction when API busy call or t…

9b1b009

…imeout * Reject split attempt early when there is an ongoing hasNext call in work item thread * TImeout hasNext call in splitAtFraction in alignment with client splitReadStreamSettings

github-actions bot added java io gcp labels Apr 27, 2024

Abacn commented Apr 27, 2024

View reviewed changes

Fix spotless

6169dd1

Abacn force-pushed the decreasehasnextcall branch from b3dd647 to 6169dd1 Compare April 27, 2024 03:28

Abacn commented Apr 27, 2024

View reviewed changes

github-actions bot added the Next Action: Reviewers label Apr 29, 2024

Abacn added this to the 2.57.0 Release milestone May 2, 2024

ahmedabu98 approved these changes May 6, 2024

View reviewed changes

...ud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOStorageReadTest.java Outdated Show resolved Hide resolved

...-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryStorageStreamSource.java Outdated Show resolved Hide resolved

Abacn added 3 commits May 6, 2024 12:28

address comments: minor logic adjust, clean up, add comment

44b33fd

add comments

409556e

Fix comments in test

4b3dd6a

Abacn merged commit 26c4ee4 into apache:master May 6, 2024
18 checks passed

Abacn deleted the decreasehasnextcall branch May 6, 2024 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress BigQuery read stream splitAtFraction when API busy call or timeout #31125

Suppress BigQuery read stream splitAtFraction when API busy call or timeout #31125

Abacn commented Apr 27, 2024 •

edited

Abacn Apr 27, 2024 •

edited

Abacn Apr 27, 2024

github-actions bot commented Apr 27, 2024

Abacn Apr 27, 2024

github-actions bot commented Apr 29, 2024

Abacn commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

ahmedabu98 left a comment

codecov-commenter commented May 6, 2024

	splitReadStreamSettings.setRetrySettings(
	splitReadStreamSettings
	.getRetrySettings()
	.toBuilder()
	.setInitialRpcTimeout(org.threeten.bp.Duration.ofSeconds(30))
	.setMaxRpcTimeout(org.threeten.bp.Duration.ofSeconds(30))
	.setTotalTimeout(org.threeten.bp.Duration.ofSeconds(30))
	.build());

Suppress BigQuery read stream splitAtFraction when API busy call or timeout #31125

Suppress BigQuery read stream splitAtFraction when API busy call or timeout #31125

Conversation

Abacn commented Apr 27, 2024 • edited

GitHub Actions Tests Status (on master branch)

Abacn Apr 27, 2024 • edited

Choose a reason for hiding this comment

Abacn Apr 27, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2024

Abacn Apr 27, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 29, 2024

Abacn commented Apr 30, 2024

github-actions bot commented Apr 30, 2024

ahmedabu98 left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 6, 2024

Codecov Report

Abacn commented Apr 27, 2024 •

edited

Abacn Apr 27, 2024 •

edited