NoahKusaba commented on code in PR #3372:
URL: https://github.com/apache/iceberg-rust/pull/3372#discussion_r4226581503
##########
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:
Agreed, made `snapshot_with_idempotency_key` private; only `commit` uses it.
I kept `IDEMPOTENCY_KEY_SUMMARY_PROPERTY` public because it names a property
stored in table metadata, so anyone reading snapshot summaries needs the name.
##########
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:
Done. The doc now says to use one key per logical write, since an append
whose key is already committed is dropped whatever its files and the commit
still returns `Ok`. It also says that after a skip, only files the table does
not reference should be deleted, because a rerun's files can be the committed
ones.
##########
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:
Agreed, 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`.
--
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]
