Repository navigation
[Java] Honor withMaxRetryJobs for bounded BigQueryIO file loads - #39938
mxtymoshyk wants to merge 4 commits into
Conversation
|
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). |
6d6879a to
1dbfb0b
Compare
|
Reminder, please take a look at this pr: @chamikaramj |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @Abacn for label java. Available commands:
|
|
Reminder, please take a look at this pr: @Abacn |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @chamikaramj for label java. Available commands:
|
1dbfb0b to
aacd74f
Compare
|
Rebased. Since release-2.77 was cut, I moved the upgrade threshold to 2.78.0 (aacd74f). |
|
The only failing check is a Pub/Sub test ( Could someone re-run it when you have a moment? |
959675b to
a731331
Compare
|
Rebased onto current master to resolve a CHANGES.md conflict, and all checks pass now, including the Pub/Sub test that failed earlier. |
9411263 to
9c03612
Compare
51f0ec6 to
3df0fbf
Compare
3df0fbf to
a0a6cd2
Compare
|
@bvolpato will you be able to continue the review here ? |
|
LGTM, but tests have to pass before merging. |
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.
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.
release-2.77 was cut before this change landed, so 2.77.0 still seeds maxRetryJobs with 1000 in every config row. Treat rows from 2.77.0 and earlier as legacy.
master now runs the nullness checker on BigQueryIOTranslation, and getMaxRetryJobs() is nullable after this change.
a0a6cd2 to
2d0413f
Compare
|
Rebased onto current master (2d0413f). All checks pass, and the change is in the 2.78.0 section of CHANGES.md. |
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.