Avoid side input when gathering unwindowed WriteFiles results#39372
Avoid side input when gathering unwindowed WriteFiles results#39372ntopousis wants to merge 5 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39372 +/- ##
============================================
- Coverage 58.03% 56.99% -1.05%
+ Complexity 13051 3632 -9419
============================================
Files 2515 1186 -1329
Lines 263756 190660 -73096
Branches 10764 3774 -6990
============================================
- Hits 153083 108658 -44425
+ Misses 104919 78518 -26401
+ Partials 5754 3484 -2270
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
13 similar comments
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
|
Assigning reviewers: R: @Abacn 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). |
1 similar comment
|
Assigning reviewers: R: @Abacn 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). |
Addresses #39370.
What changed
For bounded, unwindowed
WriteFiles, replace the globalView.asIterable()used to gather temporaryFileResults with a main-input shuffle:Void, andGroupByKey;PCollection<List<FileResult<...>>>contract;TimestampCombiner.EARLIESTinternally to retain the previous minimum element timestamp, then restore the global-default windowing strategy; andThe windowed-write path is unchanged.
FinalizeFnretains its defensive copy and invokes the existing single-call rename and cleanup protocol.FileBasedSink.WriteOperation.finalizeDestinationnow makes runner-determined shard assignment deterministic.Why
At high temporary-file cardinality, the existing global iterable side input can become a finalization bottleneck. In one Dataflow production case, approximately 90,000–100,000
FileResultrecords were gathered per large write. One materialization was about 18 MiB, while observed processing rates across the two largest finalizers were roughly 200–225 records per minute as workers repeatedly read and retried GCS data.Related prior art: #18629 reports the same high-cardinality ISM side-input pathology in BigQuery batch-load finalization. It concerns a different transform, but the finely sharded temporary-file metadata and
IsmReader/coder-size stack are closely related.Retry and finalization semantics
GroupByKeystabilizes the set of results but does not guarantee iteration order. Runner-determined shard numbers are assigned by position, so a retry after a partial rename must not remap a temporary file to a different final shard.FileBasedSink.WriteOperation.finalizeDestinationtherefore sorts unwindowed runner-determined results by their unique temporary filename at the positional shard-assignment boundary.This deliberately changes the previously arbitrary content-to-shard ordering from materialization order to lexicographic temporary-filename order. Runner-determined shard count and final filename format are unchanged. Fixed-shard assignment and the windowed-write path are unchanged.
The existing rename options and whole-directory cleanup protocol remain in place. This preserves empty and missing fixed shards, dynamic destinations, filename-policy side inputs, retry-safe temp-to-final mappings, output-filename visibility, and unwindowed temporary-directory cleanup.
Timestamp and windowing compatibility
The old side-input path exposed output-filename elements at
TIMESTAMP_MIN_VALUEwith the global-default (END_OF_WINDOW) windowing strategy. The new gather usesTimestampCombiner.EARLIESTinternally so its grouped list retains the minimum element timestamp, then restoresWindowingStrategy.globalDefault()before finalization.Without that reset,
EARLIESTwould leak throughgetPerDestinationOutputFilenames(), and a downstream aggregation applied directly to that collection could emit at the minimum timestamp rather than at the end of the global window. The patch preserves both the filename element timestamp and the externally visible windowing strategy.Compatibility and scope
The changed gather branch is selected only for non-windowed writes.
WriteFilesrejects unbounded inputs unless windowed writes are enabled, so the normal unbounded/streaming write path retains its existing two-reshuffle implementation. Dataflow replacement updates apply to streaming jobs; batch pipeline updates are not supported. The downstreamList/ListCodercontract established in #26289 also remains unchanged.A bounded
WriteFilesbranch embedded in a larger streaming pipeline is a theoretical compatibility case; maintainer guidance on whether anupdateCompatibilityVersiongate is warranted is welcome.Combine.globallyis not a drop-in simplification. With defaults, its empty-input implementation usesView.asIterable(), reintroducing the side-input pattern this change removes.withoutDefaults()avoids that view but suppresses the element needed to finalize an empty write unless it is also paired with an explicit marker.This is deliberately not parallel finalization. It still materializes one O(N) list and runs one logical finalizer. Chunked finalization requires a separate design for shard assignment, exactly-once empty-shard creation, completion barriers, and cleanup. A Dataflow-side mitigation for released SDK versions is also still worth investigating, so this PR addresses rather than auto-closes #39370.
Tests
The following local checks passed with JDK 21:
GatherTempFileResultsdoes not containView.CreatePCollectionView.FileResultset and verifies identical temp-file-to-final-shard mappings.FileBasedSinkTestclass passes.WriteFilesTestcases pass, including both empty-input cases../gradlew :sdks:java:core:spotlessCheck./gradlew :sdks:java:core:test --tests org.apache.beam.sdk.io.FileBasedSinkTest --tests org.apache.beam.sdk.io.WriteFilesTest.testUnwindowedGatherDoesNotMaterializeResultsAsSideInput --tests org.apache.beam.sdk.io.WriteFilesTest.testUnwindowedOutputFilenamesKeepGlobalDefaultWindowingStrategy./gradlew :runners:direct-java:needsRunnerTests --tests org.apache.beam.sdk.io.WriteFilesTest./gradlew formatChangesThank 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.See the Contributor Guide for more tips on how to make review process smoother.