stevenzwu commented on code in PR #16293:
URL: https://github.com/apache/iceberg/pull/16293#discussion_r4127795555
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -683,6 +685,31 @@ protected ManifestReader<DeleteFile>
newDeleteManifestReader(ManifestFile manife
return ManifestFiles.readDeleteManifest(manifest, ops.io(),
ops.current().specsById());
}
+ @VisibleForTesting
+ void setClock(Clock newClock) {
+ this.clock = newClock;
+ }
+
+ /**
+ * Generates the snapshot timestamp in milliseconds.
+ *
+ * <p>For format version 4 and above, this implements the Lamport clock
algorithm to guarantee
+ * monotonically increasing snapshot timestamps. For older format versions,
this returns the
+ * current wall clock time.
+ *
+ * @param parentSnapshot the parent snapshot on the target branch, or null
if there is no parent
+ * @return the snapshot timestamp in milliseconds
+ */
+ private long snapshotTimestampMillis(Snapshot parentSnapshot) {
Review Comment:
@RussellSpitzer The quoted snippet compares
`TableMetadata.lastUpdatedMillis` (wall clock) to the snapshot log, not
parent→child snapshot `timestamp-ms`. v4 now validates parent→child in
`addSnapshot`. I dropped the check you quoted rather than tightening it:
snapshot-log entries for new snapshots carry Lamport `timestamp-ms` and are no
longer comparable to `last-updated-ms`.
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -378,15 +379,6 @@ public String toString() {
}
last = logEntry;
}
Review Comment:
Dropped the `lastUpdatedMillis` vs last snapshot-log `ONE_MINUTE` check that
used to sit here.
These are different clocks: `TableMetadata.lastUpdatedMillis` is physical
wall clock, while snapshot `timestamp-ms` is a monotonic Lamport clock
(`max(now, parent+1)`) copied into the snapshot log for new snapshots. A
Lamport fast-forward can put the snapshot log arbitrarily ahead of wall clock,
so comparing them would reject a valid commit.
The metadata-log checks remain unchanged; both sides are wall-clock
`lastUpdatedMillis`
([L383-L402](https://github.com/apache/iceberg/blob/cead2eaf69932b67402794ac9cb9a1a1c9773035/core/src/main/java/org/apache/iceberg/TableMetadata.java#L383-L402)).
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -683,6 +685,31 @@ protected ManifestReader<DeleteFile>
newDeleteManifestReader(ManifestFile manife
return ManifestFiles.readDeleteManifest(manifest, ops.io(),
ops.current().specsById());
}
+ @VisibleForTesting
+ void setClock(Clock newClock) {
+ this.clock = newClock;
+ }
+
+ /**
+ * Generates the snapshot timestamp in milliseconds.
+ *
+ * <p>For format version 4 and above, this implements the Lamport clock
algorithm to guarantee
+ * monotonically increasing snapshot timestamps. For older format versions,
this returns the
+ * current wall clock time.
+ *
+ * @param parentSnapshot the parent snapshot on the target branch, or null
if there is no parent
+ * @return the snapshot timestamp in milliseconds
+ */
+ private long snapshotTimestampMillis(Snapshot parentSnapshot) {
+ long now = clock.millis();
+ if (base.formatVersion() >=
TableMetadata.MIN_FORMAT_VERSION_MONOTONIC_TIMESTAMPS
+ && parentSnapshot != null) {
+ return Math.max(now, parentSnapshot.timestampMillis() + 1);
Review Comment:
Done in the latest commit: Java always stamps `max(now, parent+1)`. Spec
validation remains v4-only in `addSnapshot`.
##########
core/src/test/java/org/apache/iceberg/TestSnapshotProducer.java:
##########
@@ -276,4 +279,89 @@ public void
testWriteManifestsWithInvalidParallelismThrows() {
executor.shutdownNow();
}
}
+
Review Comment:
Added `transactionCommitsProduceMonotonicTimestamps` and
`transactionRetryFastForwardsTimestampsPastConflictingSnapshot` (retry after a
conflicting snapshot whose timestamp is ahead of the txn's stale clock).
--
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]
