[Iceberg CDC] Finish wiring CDC source together and add external API - #39600
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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). |
|
|
||
| // process one snapshot at a time and produce batches of changelog scan tasks. | ||
| // tasks are emitted to three outputs: | ||
| // 1. unidirectional tasks: we know these won't have any updates |
There was a problem hiding this comment.
How about deletes ?
Did you mean "these only contain inserts" ?
There was a problem hiding this comment.
"unidirectional tasks" refers to a group of tasks where we are certain there is no insert-delete collision.
For example, it may contain ONLY inserts, or ONLY deletes. or it may contain inserts and deletes that don't overlap in PK
| PCollection<Row> smallBidirectionalCdcRows = | ||
| changelogTasks | ||
| .get(SMALL_BIDIRECTIONAL_TASKS) | ||
| .apply("Redistribute Small Bidirectional Changes", Redistribute.arbitrarily()) |
There was a problem hiding this comment.
Seems like we have a shuffle here ?
There was a problem hiding this comment.
Yeah but this is before we apply any ValueKind metadata. Same with the CoGroupByKey in the large bi-directional path -- it's before any ValueKind metadata gets added
| Window<KV<CdcRowDescriptor, Row>> keyedWindowing = | ||
| Window.<KV<CdcRowDescriptor, Row>>into(new SnapshotWindowFn()) | ||
| .triggering(AfterWatermark.pastEndOfWindow()) | ||
| .withAllowedLateness(Duration.ZERO) |
There was a problem hiding this comment.
Is dropping late data the correct approach here ?
There was a problem hiding this comment.
The watermark here is actually controlled by us (in WatchForSnapshotsSdf). We only advance it past a snapshot's timestamp after emitting that snapshot, so in the normal flow there's no "late" data. Timestamps represent snapshot commit time
There was one exception I just fixed. On empty polls we bump the watermark to now() - pollInterval so downstream windows don't stall while the table is quiet. But if snapshots are committed and only discovered much later (e.g. a catalog outage), that bump could run ahead of their commit times, and they'd arrive late and get silently dropped. Fixed this edge case by setting output timestamp = max(commitTs, currentWatermark).
| KeyedPCollectionTuple.of(INSERTS, keyedInserts) | ||
| .and(DELETES, keyedDeletes) | ||
| .apply("CoGroupBy Primary Key", CoGroupByKey.create()) | ||
| .apply("Resolve Delete-Insert Pairs", ParDo.of(new ResolveChanges(scanConfig))) |
There was a problem hiding this comment.
I suspect this is something we can do at the framework level later right ? (Without having to do this kind of optimizations per source)
There was a problem hiding this comment.
This logic is written just for Iceberg actually. Iceberg doesn't have any native concept of an "update", so we (the engine) have to figure it out ourselves
There was a problem hiding this comment.
But it can be mostly re-used if we encounter another source with similar behavior
| } | ||
|
|
||
| @Override | ||
| protected int nonPkHash(Row element) { |
There was a problem hiding this comment.
How expensive is this ? Seems like we are hashing pretty much all fields here of all elements ? (not a blocker)
There was a problem hiding this comment.
Well we need to compare all fields to correctly determine if it's an update. This is only done for records that fall in the overlap range. Other records skip this entirely
| } | ||
|
|
||
| @Override | ||
| protected boolean nonPkEquals(Row delete, Row insert) { |
There was a problem hiding this comment.
Ditto (might be good to do a perf analysis after the PR is submitted)
There was a problem hiding this comment.
See comment above. We only do this equality comparison on hash collision, so it's even more rare.
There was a problem hiding this comment.
Perf analysis could be warranted though
| * require a row-lineage table. The changelog metadata columns come from the emitted change kind | ||
| * and snapshot context and are appended when final Beam rows are emitted. | ||
| * | ||
| * <p>This option is only valid {@link #withCdc()}. |
There was a problem hiding this comment.
Let's fail if the incorrect combination is set.
There was a problem hiding this comment.
It does in IcebergScanConfig#validate
| if (ts.isBefore(watermark.currentWatermark())) { | ||
| // The watermark already moved past this snapshot's commit time (e.g. the idle bump ran | ||
| // ahead of a slow discovery). Use the current watermark so the snapshot is not dropped | ||
| ts = watermark.currentWatermark(); |
There was a problem hiding this comment.
Probably there still a race (extremely rare) between we setting this here and runner firing for the current watermark, right ?
There was a problem hiding this comment.
I don't think so, IIUC the watermark only advances when the bundle commits, along with the emmitted elements. and those same in-flight elements hold back the watermark until they arrive downstream, so a downstream window can't fire until they all arrive
| abstract @Nullable String getWatermarkColumnTimeUnit(); | ||
|
|
||
| @SchemaFieldDescription( | ||
| "Maximum expected snapshot discovery delay in seconds. While idle, the source may advance " |
There was a problem hiding this comment.
Did you intend to remove this knob ?
There was a problem hiding this comment.
Yeah we no longer need it after the fix mentioned in #39600 (comment)
Only had it to allow customers to control what we would consider "late". but now even if the catalog is slow/down, we wouldn't consider the missed snapshots as late. We just weave them in and attach current watermark as timestamp
…pache#39600) * wrap up * trigger ITs and add to CHANGES.md * fix late snapshot edge case * update resolution optimization --------- Co-authored-by: Ahmed Abualsaud <ahmedabualsaud@MacBook-Pro-2.local>
…pache#39600) * wrap up * trigger ITs and add to CHANGES.md * fix late snapshot edge case * update resolution optimization --------- Co-authored-by: Ahmed Abualsaud <ahmedabualsaud@MacBook-Pro-2.local>
Finalizes the IcebergIO CDC streaming source. Introduces top-level IncrementalChangelogSource to replace IncrementalAppendSource.
Reads a table's changelog one snapshot at a time and emits net per-snapshot changes as CDC rows. Bi-directional groups too large to resolve in memory are windowed per snapshot (SnapshotWindowFn), grouped by primary key, and reconciled by ResolveChanges, while small groups continue to resolve locally (LocalResolveDoFn) without a shuffle.
Adds user-facing options for CDC metadata columns, a custom watermark column with its time unit, and a maximum snapshot discovery delay.
Removes the now-unused IncrementalScanSource, ReadFromTasks, and WatchForSnapshots.
Fixes #38831
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.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 to see a list of phrases to trigger workflows.