mxtymoshyk commented on code in PR #39938:
URL: https://github.com/apache/beam/pull/39938#discussion_r3904177130
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java:
##########
@@ -4227,10 +4236,15 @@ private <DestinationT> WriteResult continueExpandTyped(
batchLoads.setMaxFilesPerPartition(getMaxFilesPerPartition());
batchLoads.setMaxBytesPerPartition(getMaxBytesPerPartition());
- // When running in streaming (unbounded mode) we want to retry failed
load jobs
- // indefinitely. Failing the bundle is expensive, so we set a fairly
high limit on retries.
- if (IsBounded.UNBOUNDED.equals(input.isBounded())) {
- batchLoads.setMaxRetryJobs(getMaxRetryJobs());
+ // an explicit withMaxRetryJobs applies to batch and streaming alike.
left unset, streaming
+ // retries a failed load job far more often than batch does: failing
the bundle in streaming
+ // is expensive, so we would rather keep retrying the job than hand
the work back to the
+ // runner. batch leaves BatchLoads on its own lower default
+ Integer maxRetryJobs = getMaxRetryJobs();
Review Comment:
Good catch, you're right. Before this change `BigQueryIO.write()` seeded the
builder with `.setMaxRetryJobs(1000)`, so `toConfigRow` wrote 1000 into every
row whether or not the pipeline ever called `withMaxRetryJobs()`. The new code
reads any non-null value as an explicit request, so a batch `FILE_LOADS`
pipeline rebuilt from an older row would have gone from 3 retries to 1000.
Fixed in `fromConfigRow`. A row from before 2.77.0 holding exactly 1000
carries no information about intent, because that was the value either way, so
it is now left unset and each mode falls back to the number it used before the
upgrade: 3 for bounded, 1000 for unbounded. Any other value was chosen
deliberately and is still carried over. Rows from 2.77.0 and later only contain
the field when the pipeline set it, so an explicit `withMaxRetryJobs(1000)`
survives there.
Two things worth flagging. A pre-2.77.0 bounded pipeline that did call
`withMaxRetryJobs(500)` will now get 500 instead of 3, but that is the bug this
PR is fixing and it is covered by the CHANGES.md entry. And since
`fromConfigRow` treats a missing `updateCompatibilityVersion` as 2.53.0, a
current pipeline that explicitly asks for 1000 and goes through this path
without that option set will also be treated as unset. That affects only the
single value 1000, and it matches how the rest of this method already handles
version defaults.
Added three cases to `BigQueryIOTranslationTest` covering the legacy 1000, a
legacy value that is not a default, and a 2.77.0 row. I checked they fail if
the new guard is removed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
