rdblue commented on code in PR #4071:
URL: https://github.com/apache/iceberg/pull/4071#discussion_r846831120
##########
core/src/main/java/org/apache/iceberg/MetadataUpdate.java:
##########
@@ -227,31 +228,64 @@ public String name() {
@Override
public void applyTo(TableMetadata.Builder metadataBuilder) {
- // TODO: this should be generalized when tagging is supported
- metadataBuilder.removeBranch(name);
+ metadataBuilder.removeRef(name);
}
}
class SetSnapshotRef implements MetadataUpdate {
private final String name;
- private final long snapshotId;
-
- public SetSnapshotRef(String name, long snapshotId) {
+ private final Long snapshotId;
+ private final SnapshotRefType type;
+ private Integer minSnapshotsToKeep;
+ private Long maxSnapshotAgeMs;
+ private Long maxRefAgeMs;
+
+ public SetSnapshotRef(String name, Long snapshotId, SnapshotRefType type,
Integer minSnapshotsToKeep,
+ Long maxSnapshotAgeMs, Long maxRefAgeMs) {
this.name = name;
this.snapshotId = snapshotId;
+ this.type = type;
+ this.minSnapshotsToKeep = minSnapshotsToKeep;
+ this.maxSnapshotAgeMs = maxSnapshotAgeMs;
+ this.maxRefAgeMs = maxRefAgeMs;
}
public String name() {
return name;
}
+ public String type() {
+ return type.name().toLowerCase(Locale.ROOT);
+ }
+
public long snapshotId() {
return snapshotId;
}
+ public Integer minSnapshotsToKeep() {
+ return minSnapshotsToKeep;
+ }
+
+ public Long maxSnapshotAgeMs() {
+ return maxSnapshotAgeMs;
+ }
+
+ public Long maxRefAgeMs() {
+ return maxRefAgeMs;
+ }
+
@Override
public void applyTo(TableMetadata.Builder metadataBuilder) {
- metadataBuilder.setBranchSnapshot(snapshotId, name);
+ if (type == SnapshotRefType.BRANCH) {
Review Comment:
For `ManageSnapshots` updates, I think the right logic for this method is to
call `setRef` for both tags and branches. This should create a ref from the
settings and use `setRef` to update the table state.
I think the reason why you're calling `setBranchSnapshot` is that method is
what was previously used by this for normal branch commits (specifically, to
main). That does extra work and changing this probably caused test failures,
which is why I think you're doing it this way. But, this is going to discard
any branch changes to ref retention settings.
What we need to do is to make sure that `setRef` handles the tasks that
`setBranchSnapshot` is doing so that updating through that path is correct. I
think the right solution is to update `setBranchSnapshot` to use `setRef`.
Right now, `setBranchSnapshot` calls `setBranchSnapshotInternal` with the
target `Snapshot`. That checks to make sure the change is a noop, checks the
sequence number, updates the snapshot history if the branch is main, then
builds the new ref, and updates refs and changes. I think you can call `setRef`
directly here if you move the history changes into `setRef` --- which makes
sense anyway because calling `replaceBranch("main", ...)` should update table
history.
Then you should be able to just call `setRef` here. That way when a branch
commit comes in, there are checks that eventually create a `SetSnapshotRef`
that makes the changes. We just need to make the same final changes, not make
sure the same builder methods (`setBranchSnapshot`) are called.
--
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]
