Repository navigation
feat(transaction): add idempotency key to fast append - #3372
NoahKusaba wants to merge 3 commits into
Conversation
|
cc @mbutrovich @comphead @gabotechs This is a major blocker for correct inserts into iceberg for distributed engines. |
comphead
left a comment
There was a problem hiding this comment.
Thanks for the PR. Suggestions below, none blocking.
Smaller points:
test_rerun_finds_its_key_below_later_snapshotsandtest_snapshot_with_idempotency_key_finds_each_keywalk the same two-snapshot history, andalready_committedonly forwards to the helper. The first one can go.test_append_without_files_records_its_keyasserts the whole summary map, including thetotal-*zeros, so an unrelated summary change breaks it. Asserting theIDEMPOTENCY_KEY_SUMMARY_PROPERTYentry is enough.- No test covers a skipped append next to a sibling action that still has updates, for example
update_table_propertiesin the same transaction. That is the case where the new early return indo_commitmust not fire. A short test besideappendinidempotency_key_testswould pin it. - A skip is silent, and the caller gets
Okwith no signal. Atracing::info!with the key and the matching snapshot id would make a dropped append diagnosable. Spark logs the same event inSparkWrite("Skipping epoch ... as it was already committed"). snapshot_properties()shares its name with the field but returns the field plus the key. A name likesummary_properties()avoids the confusion.
| /// whether this append was the one committed, see | ||
| /// [`snapshot_with_idempotency_key`]. | ||
| /// | ||
| /// An append with no data files still commits a snapshot recording `key`, |
There was a problem hiding this comment.
A zero-file append only works today because SnapshotProducer::produce_manifests accepts a properties-only append (snapshot.rs:345). That branch is marked as a workaround to clean up (#1548). If the cleanup removes it, an append with a key and no files would likely start failing with PreconditionFailed, which contradicts this paragraph.
Is the empty snapshot needed, for example so a caller can use the key as a completion marker? A retried zero-file job adds no data, so there is nothing to protect from duplication. If the marker is wanted, please note the dependency in the TODO in snapshot.rs. If not, skipping a zero-file append would avoid an empty snapshot per job and let this paragraph and test_append_without_files_records_its_key go.
There was a problem hiding this comment.
I originally kept it in as AI pointed out consistency with Flink's write design, but I'm overall indifferent and you're right the empty snapshot isn't needed. A keyed append with no data files and no snapshot properties now commits nothing and returns Ok, so nothing new depends on the #1548 workaround. With caller-set snapshot properties it still commits as before, so the key never drops them. The paragraph and test_append_without_files_records_its_key are replaced by test_append_without_files_commits_nothing and test_append_without_files_but_with_properties_commits.
|
Sorry for responding late (having a busy week) @comphead and thanks for the review, all addressed in db9d1a5:
|
Which issue does this PR close?
TransactionActionpublic).What changes are included in this PR?
An engine that reruns a task can commit the same append twice. Checking for an earlier commit before
Transaction::commitcosts an extra table load and still races, becausedo_commitreloads the table and rebuilds the actions.FastAppendAction::with_idempotency_key(key)recordskeyin the snapshot summary and checks the refreshed table for it on every commit attempt, skipping the append if a snapshot onmainalready has it. A racing loser fails its ref requirement, retries, finds the key and skips. Flink's committer uses the same pattern withflink.job-id.Not obvious from the diff:
do_commitno longer callsupdate_tablewhen no action produced updates or requirements, as in Java. This affects every transaction: for example,expire_snapshotswith nothing to expire no longer writes a metadata file.mainare not found.Are these changes tested?
Yes, unit tests: reruns from a stale table and with the same files, the race (replayed through a mock catalog in sequence), appends without files with and without snapshot properties, a skipped append next to an action that still commits, and a commit without updates.
AI Disclosure
Drafted with Claude Code; I reviewed it and ran the tests, clippy and fmt.