comphead commented on code in PR #3372:
URL: https://github.com/apache/iceberg-rust/pull/3372#discussion_r4212115645
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -66,6 +93,35 @@ impl FastAppendAction {
self
}
+ /// Make this append idempotent under `key`.
+ ///
+ /// The key is recorded in the new snapshot's summary under
+ /// [`IDEMPOTENCY_KEY_SUMMARY_PROPERTY`]. On every commit attempt,
including
+ /// retries against a refreshed table, the action first walks the ancestry
of
+ /// the `main` branch; if a snapshot already carries `key`, the append
+ /// produces no updates and the transaction commits nothing for it. The
+ /// check comes before the added files are validated, so a rerun appending
+ /// the files the first run committed is skipped, not rejected as a
+ /// duplicate.
+ ///
+ /// This lets a caller safely rerun a commit that may already have
Review Comment:
Please state two consequences for callers here. Attempts under one key must
append equivalent data, because a rerun with different files is skipped without
any signal (`test_rerun_from_a_stale_table_commits_nothing` relies on this).
And the files of a skipped attempt are not referenced by the table, but they
can be the very files the winning attempt committed when the paths match, so
cleanup after a skip needs care.
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -66,6 +93,35 @@ impl FastAppendAction {
self
}
+ /// Make this append idempotent under `key`.
+ ///
+ /// The key is recorded in the new snapshot's summary under
+ /// [`IDEMPOTENCY_KEY_SUMMARY_PROPERTY`]. On every commit attempt,
including
+ /// retries against a refreshed table, the action first walks the ancestry
of
+ /// the `main` branch; if a snapshot already carries `key`, the append
+ /// produces no updates and the transaction commits nothing for it. The
+ /// check comes before the added files are validated, so a rerun appending
+ /// the files the first run committed is skipped, not rejected as a
+ /// duplicate.
+ ///
+ /// This lets a caller safely rerun a commit that may already have
+ /// succeeded (for example a task retried by a distributed engine). If two
+ /// attempts race, the loser fails its `main` ref requirement, retries
+ /// against the refreshed table, sees the winner's key and skips. To learn
+ /// whether this append was the one committed, see
+ /// [`snapshot_with_idempotency_key`].
+ ///
+ /// An append with no data files still commits a snapshot recording `key`,
Review 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.
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -22,18 +22,44 @@ use async_trait::async_trait;
use uuid::Uuid;
use crate::error::Result;
-use crate::spec::{DataFile, ManifestEntry, ManifestFile, Operation};
+use crate::spec::{DataFile, ManifestEntry, ManifestFile, Operation,
SnapshotRef};
use crate::table::Table;
use crate::transaction::snapshot::{
DefaultManifestProcess, SnapshotProduceOperation, SnapshotProducer,
};
use crate::transaction::{ActionCommit, TransactionAction};
+use crate::util::snapshot::ancestors_of;
+
+/// Snapshot summary property that records the idempotency key of a fast
append.
+/// See [`FastAppendAction::with_idempotency_key`].
+pub const IDEMPOTENCY_KEY_SUMMARY_PROPERTY: &str =
"iceberg-rust.idempotency-key";
+
+/// Returns the snapshot on the `main` branch of `table` whose summary records
+/// idempotency key `key`, if any.
+///
+/// After committing a [`FastAppendAction`] with
+/// [`FastAppendAction::with_idempotency_key`], this finds the snapshot holding
+/// the key, whether this commit or an earlier one created it. To tell the two
+/// apart, record a value unique to the commit call with
+/// [`FastAppendAction::set_snapshot_properties`] and compare it with the
+/// returned snapshot's summary.
+pub fn snapshot_with_idempotency_key(table: &Table, key: &str) ->
Option<SnapshotRef> {
Review Comment:
Does a downstream user already need `snapshot_with_idempotency_key` and
`IDEMPOTENCY_KEY_SUMMARY_PROPERTY` to be public? By its own docs the helper
cannot tell a caller whether its append won. The caller has to add a second
unique property and compare. If nothing needs them yet, `pub(crate)` keeps the
new API to `with_idempotency_key`, and widening later is not a breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
