amogh-jahagirdar commented on code in PR #4071:
URL: https://github.com/apache/iceberg/pull/4071#discussion_r841382268
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1007,6 +1007,49 @@ public Builder setBranchSnapshot(long snapshotId, String
branch) {
return this;
}
+ public Builder setTag(String tag, SnapshotRef ref) {
+ SnapshotRef existingRef = refs.get(tag);
+ if (existingRef != null && existingRef.equals(ref)) {
+ return this;
+ }
+ long snapshotId = ref.snapshotId();
+ Snapshot snapshot = snapshotsById.get(snapshotId);
+ ValidationException.check(snapshot != null, "Cannot set %s to unknown
snapshot: %s", tag, snapshotId);
+ ValidationException.check(ref.isTag(), "Cannot create tag: %s is a
branch", tag);
+ refs.put(tag, ref);
+ changes.add(new MetadataUpdate.SetSnapshotRef(tag, snapshotId,
SnapshotRefType.TAG, null, null, null));
+ return this;
+ }
+
+ public Builder setBranch(String branch, SnapshotRef ref) {
+ SnapshotRef existingRef = refs.get(branch);
+ if (existingRef != null && existingRef.equals(ref)) {
+ return this;
+ }
+ long snapshotId = ref.snapshotId();
+ Snapshot snapshot = snapshotsById.get(snapshotId);
+ ValidationException.check(snapshot != null, "Cannot set %s to unknown
snapshot: %s", branch, snapshotId);
+ ValidationException.check(ref.isBranch(), "Cannot create branch: %s is a
tag", branch);
+ refs.put(branch, ref);
+ MetadataUpdate.SetSnapshotRef refUpdate =
+ new MetadataUpdate.SetSnapshotRef(branch, snapshotId,
SnapshotRefType.BRANCH);
+ boolean newRef = existingRef == null;
+ boolean updateMinSnapshots = newRef ||
!Objects.equal(ref.minSnapshotsToKeep(), existingRef.minSnapshotsToKeep());
Review Comment:
I thought the intent for the MetadataUpdate was to encapsulate as minimal
changes as possible?
So for example if we have Branch foo with {snapshotId: 1,
minSnapshotsToKeep: 1, maxSnapshotAgeMs: 100, maxRefAgeMs: 100}. If a SetBranch
was done on "foo" with the updated reference {snapshotId: 3,
minSnapshotsToKeep: 1, maxSnapshotAgeMs: 200, maxRefAgeMs: 100}. The only
fields which changed are the snapshot ID and the maxSnapshotAgeMs, so the
SetSnapshotRef would have just {snapshotId: 3, maxSnapshotAgeMs: 200}. Then
when applyTo is called we can set those fields accordingly.
Or is it the desired behavior that even a single non-null value in the
incoming ref implies we need to set every field in the update (this seemed to
go against the goal of Metadata update being minimal and idempotent)?
--
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: dev-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
