[Java] Honor withMaxRetryJobs for bounded BigQueryIO file loads - #39938
[Java] Honor withMaxRetryJobs for bounded BigQueryIO file loads#39938mxtymoshyk wants to merge 2 commits into
Conversation
BigQueryIO.Write only forwarded maxRetryJobs to BatchLoads when the input was unbounded, so every batch pipeline used BatchLoads' own default of 3 retries and withMaxRetryJobs did nothing at all. A user who asked for fewer retries on a permanently failing load job, or for more, got neither and only saw "reached max retries: 3" after the write had given up. The value now reaches BatchLoads in both modes. Write.getMaxRetryJobs becomes a nullable Integer that BigQueryIO.write() leaves unset, so an absent setting can still fall back to the two different defaults the two modes have always had: 3 for bounded, 1000 for unbounded. Also documents on withMaxRetryJobs that the setting applies to FILE_LOADS only and what happens when it is not called.
|
Assigning reviewers: R: @chamikaramj for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| // 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(); |
There was a problem hiding this comment.
could this preserve old bounded default when rebuilding a write from a legacy config row? older versions always serialized max_retry_jobs=1000, even when withMaxRetryJobs() was never called. fromConfigRow() restores that non-null value, so this branch appears to treat it as explicit and could change bounded FILE_LOADS retries from 3 to 1000.
There was a problem hiding this 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.
BigQueryIO.write() used to seed maxRetryJobs with 1000 whether or not the pipeline ever called withMaxRetryJobs, so every config row written by an older SDK carries that value. fromConfigRow restored it, and the new expand() would have read it as an explicit setting, raising bounded FILE_LOADS retries from 3 to 1000 on upgrade. A pre-2.77.0 row holding exactly 1000 says nothing about what the user asked for, so drop it and let each mode fall back to the default it used before: 3 for bounded and 1000 for unbounded. Any other value was chosen deliberately and is carried over.
2 participants
BigQueryIO.Write.withMaxRetryJobswas silently ignored by every bounded (batch) pipeline.continueExpandTypedonly forwarded the value toBatchLoadswhen the input wasIsBounded.UNBOUNDED, so a batch write always usedBatchLoads.DEFAULT_MAX_RETRY_JOBS(3) no matter what the user asked for. The failure surfaces only asreached max retries: 3in the exception text after the job has already given up.The setting is now honored in both modes. Pipelines that never call
withMaxRetryJobskeep exactly the limits they have today: 3 for bounded, 1000 for unbounded.The Javadoc said only "If set, this will set the max number of retry of batch load jobs." It now states that the setting applies to
FILE_LOADSonly and what the two defaults are, which was the second half of the issue.fixes #28281
How the default is preserved
Write.getMaxRetryJobs()becomes@Nullable Integerand is no longer given a value inBigQueryIO.write(). Null means "user never asked", which is what makes it possible to honor an explicit value in batch without also raising the batch default from 3 to 1000. This mirrorsgetMaxFilesPerBundle/getMaxFileSizein the sameWriteclass -- nullable, absent from thewrite()defaults, and applied under a null check at expansion time.The two default values now live next to each other as
BatchLoads.DEFAULT_MAX_RETRY_JOBSandBatchLoads.DEFAULT_MAX_RETRY_JOBS_UNBOUNDED, so the reason streaming retries far more than batch is visible in one place.Notes for reviewers
withMaxRetryJobs, and if they do call it on a batch pipeline they currently get nothing.maxRetryJobsonly ever reaches theFILE_LOADSbranch ofcontinueExpandTyped, so the "only applies toMethod#FILE_LOADS" sentence in the Javadoc is accurate rather than a guess.BigQueryIOTranslationneeded no change:max_retry_jobswas already declared withaddNullableInt32Field,toConfigRowcan put a null, andfromConfigRowalready skips a null.BigQueryIOTranslationTestpasses unchanged.withMaxRetryJobs(int)keeps its signature. Only the package-private AutoValue accessor and setter changed type.testWriteFailedJobsnow assertsreached max retries: 3rather than the bare substring. That pins the bounded default so a future change cannot quietly move it.testWriteFailedJobsRespectsMaxRetryJobsWhenBoundedreuses the existingtestWriteFailedJobsharness (CREATE_NEVERagainst a table that does not exist, so every load job attempt fails) and setswithMaxRetryJobs(1).PendingJobputs the applied limit into its own failure message, so the assertion reads the value that actually reachedBatchLoads. A low value also keeps the test fast, since each retry costs a backoff sleep.:sdks:java:io:google-cloud-platform.BigQueryIOWriteTest.testWriteFailedJobs*,testMaxRetryJobs*and the whole ofBigQueryIOTranslationTestpass (21 run, 0 failures). Restoring only the oldif (IsBounded.UNBOUNDED...)guard makestestWriteFailedJobsRespectsMaxRetryJobsWhenBounded[0]fail whiletestWriteFailedJobsstill passes, so the new test covers the fix and nothing else.