Repository navigation
Conversation
Batch writes now grow their 500/50/5 ramp-up budget from one start time shared by the whole pipeline, provided as a singleton side input, instead of from each DoFn instance's first write. This follows DatastoreV1, so DoFn instances created later (autoscaling, retried bundles) no longer restart the ramp-up. The side input is omitted when updateCompatibilityVersion is below 2.78.0. Addresses apache#27170. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BofSrVTSEGJvEzsSNXfU2o
|
Assigning reviewers: R: @ahmedabu98 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). |
|
Reminder, please take a look at this pr: @ahmedabu98 |
|
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: @kennknowles for label java. Available commands:
|
| */ | ||
| private static @Nullable PCollectionView<Instant> createRampUpStartView( | ||
| PCollection<?> input, JodaClock clock) { | ||
| if (StreamingOptions.updateCompatibilityVersionLessThan( |
There was a problem hiding this comment.
The way we should use this flag is that we should clone the prior expand method to expand_2_77(...).
- The
expand_2_77(...)method will always have an unchanged topology. - The new
expanddelegates toexpand_2_77in its first line(s) and after that has the new, totally separate, topology. - This eliminate the "action at a distance" of this return value being null causing the graph topology to be altered latter.
Check other uses of this pipeline option for examples. Particularly a simple case I can think of is Reshuffle.
They can share sub-transforms or helper functions, but the forking based on update compatibility should happen at the highest level. This is a case where redundancy is a benefit, and pushing the cyclomatic complexity to the earliest possible point is the way to do it.
There was a problem hiding this comment.
Thanks, done in 53a7ca9. Each batch write transform keeps the previous expansion unchanged in expand_2_78_0, and expand delegates to it in its first lines when updateCompatibilityVersion is 2.78.0 or earlier, as Reshuffle does. The new path no longer has a nullable side input.
I named it _2_78_0 rather than _2_77 because 2.78.0 was cut without this change, so the new shape starts with 2.79.0. I also merged master, which now includes #40390.
|
Reminder, please take a look at this pr: @kennknowles @ahmedabu98 |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BofSrVTSEGJvEzsSNXfU2o
Per review, the previous batch write expansion is kept unchanged in expand_2_78_0, and expand delegates to it when updateCompatibilityVersion is 2.78.0 or earlier, instead of threading a nullable side input through the new expansion. The cutoff moves to 2.79.0 since 2.78.0 was cut without this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BofSrVTSEGJvEzsSNXfU2o
|
The failing job seems to be a flaky one, unrelated to this PR: it's |
Addresses #27170 (alleviates it, does not fix it).
What
FirestoreIO batch writes now grow their ramp-up budget from one pipeline-wide start time, instead of from each DoFn instance's first write.
FirestoreV1.BatchWriteWithSummaryandBatchWriteWithDeadLetterQueuecreate a singleton side input holdingclock.instant(), computed once per pipeline (Create→MapElements→View.asSingleton()), and pass it to the writeParDo.FirestoreV1WriteFn.BaseBatchWriteFn.processElementreads the side input and hands it toRpcQos.setRampUpStart(Instant).RpcQosImpl.WriteRampUpuses that instant as its ramp-up start. If it's never set, behavior is unchanged: the ramp-up starts lazily at the first write.Why
#27170 describes how slow the write ramp-up is for small jobs. On top of the low starting budget (
500 / hintMaxNumWorkers, 1 write/s with the default hint), eachRpcQosinstance (one per DoFn instance, created in@Setup) starts its own ramp-up clock at its first write. Every DoFn instance created later starts again at the base budget. This happens on autoscaled workers, when concurrency grows, and after a failed bundle discards its DoFn instances. Time the pipeline spends before its first write doesn't count either.With a shared start time:
Trade-off, the same one DatastoreIO accepts: if the first write happens long after the pipeline starts, the budget has already grown, so those writes get less ramp-up protection.
Where it comes from
This copies the approach DatastoreIO already uses.
DatastoreV1.Mutate.expandcreates a "Generate start timestamp" side input (Create.of("side input")→MapElements.via(s -> Instant.now())→View.asSingleton()) and passes it toRampupThrottlingFn. ItsprocessElementreadsc.sideInput(firstInstantSideInput)and computes the budget relative to that instant. This PR applies the same pattern to the Firestore write path. The Firestore ramp-up lives insideRpcQosImplrather than in a separate DoFn, so the instant is passed into it.Pipeline update compatibility
The write transforms gain the side-input transforms, which changes the pipeline graph. Following the existing pattern (for example
Reshuffle), the previous expansion is kept unchanged inexpand_2_78_0, andexpanddelegates to it when--updateCompatibilityVersionis 2.78.0 or earlier. Pipelines that need to update a running job from an older SDK can set it and keep the previous shape and behavior. The cutoff is 2.79.0 because 2.78.0 was cut without this change.Why this alleviates rather than fixes the referenced issue
The rate formula is unchanged. A small job that writes from the start still begins at
500 / hintMaxNumWorkerswrites per second and grows by 50% every 5 minutes, so with the default hint it is still slow for the first 10–15 minutes. This PR removes the extra slowdown from per-instance restarts and makes Firestore behave like Datastore. Changing the default growth is left for a follow-up.Testing
RpcQosTest.rampUp_growsFromProvidedStart: with a start 20 minutes before the first write, the first batch is sized from the grown budget (3), not the base budget (1).FirestoreV1FnBatchWriteWithSummaryTest.rampUpStartIsReadFromSideInput: the DoFn reads the side input and passes it toRpcQos.setRampUpStart.FirestoreV1BatchWriteRampUpStartTest: both write transforms add the side input to theirbatchWriteParDo; withupdateCompatibilityVersion=2.78.0they keep the previous shape, and with 2.79.0 they add it.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith 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 for a list of workflows and how to trigger them.