From 6fc5be738d317eb1e8a5b525f5b8e40841dee678 Mon Sep 17 00:00:00 2001 From: Amogh Jahagirdar Date: Mon, 27 Nov 2023 11:24:39 -0800 Subject: [PATCH] API, Core: Fix naming in fastForwardBranch/replaceBranch APIs (#9134) --- .../org/apache/iceberg/ManageSnapshots.java | 22 ++++++------ .../org/apache/iceberg/SnapshotManager.java | 8 ++--- .../UpdateSnapshotReferencesOperation.java | 35 ++++++++++--------- .../apache/iceberg/TestSnapshotManager.java | 28 ++++++++++++--- 4 files changed, 57 insertions(+), 36 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/ManageSnapshots.java b/api/src/main/java/org/apache/iceberg/ManageSnapshots.java index 2fa60472da5e..986bbb6f5809 100644 --- a/api/src/main/java/org/apache/iceberg/ManageSnapshots.java +++ b/api/src/main/java/org/apache/iceberg/ManageSnapshots.java @@ -163,26 +163,26 @@ default ManageSnapshots createBranch(String name) { ManageSnapshots replaceBranch(String name, long snapshotId); /** - * Replaces the branch with the given name to point to the source snapshot. The source branch will - * remain unchanged, the target branch will retain its retention properties. + * Replaces the {@code from} branch to point to the {@code to} snapshot. The {@code to} will + * remain unchanged, and {@code from} branch will retain its retention properties. * - * @param name Branch to replace - * @param source Source reference for the target to be replaced with + * @param from Branch to replace + * @param to The branch {@code from} should be replaced with * @return this for method chaining */ - ManageSnapshots replaceBranch(String name, String source); + ManageSnapshots replaceBranch(String from, String to); /** - * Performs a fast-forward of the given target branch up to the source snapshot if target is an - * ancestor of source. The source branch will remain unchanged, the target branch will retain its + * Performs a fast-forward of {@code from} up to the {@code to} snapshot if {@code from} is an + * ancestor of {@code to}. The {@code to} will remain unchanged, and {@code from} will retain its * retention properties. * - * @param name Branch to fast-forward - * @param source Source reference for the target to be fast forwarded to + * @param from Branch to fast-forward + * @param to Ref for the {@code from} branch to be fast forwarded to * @return this for method chaining - * @throws IllegalArgumentException if the target branch is not an ancestor of source + * @throws IllegalArgumentException if {@code from} is not an ancestor of {@code to} */ - ManageSnapshots fastForwardBranch(String name, String source); + ManageSnapshots fastForwardBranch(String from, String to); /** * Updates the minimum number of snapshots to keep for a branch. diff --git a/core/src/main/java/org/apache/iceberg/SnapshotManager.java b/core/src/main/java/org/apache/iceberg/SnapshotManager.java index c9774f3b929b..bb7ca4b11c11 100644 --- a/core/src/main/java/org/apache/iceberg/SnapshotManager.java +++ b/core/src/main/java/org/apache/iceberg/SnapshotManager.java @@ -137,14 +137,14 @@ public ManageSnapshots replaceBranch(String name, long snapshotId) { } @Override - public ManageSnapshots replaceBranch(String name, String source) { - updateSnapshotReferencesOperation().replaceBranch(name, source); + public ManageSnapshots replaceBranch(String from, String to) { + updateSnapshotReferencesOperation().replaceBranch(from, to); return this; } @Override - public ManageSnapshots fastForwardBranch(String name, String source) { - updateSnapshotReferencesOperation().fastForward(name, source); + public ManageSnapshots fastForwardBranch(String from, String to) { + updateSnapshotReferencesOperation().fastForward(from, to); return this; } diff --git a/core/src/main/java/org/apache/iceberg/UpdateSnapshotReferencesOperation.java b/core/src/main/java/org/apache/iceberg/UpdateSnapshotReferencesOperation.java index b87bac2f014f..2c3c6c1f7e10 100644 --- a/core/src/main/java/org/apache/iceberg/UpdateSnapshotReferencesOperation.java +++ b/core/src/main/java/org/apache/iceberg/UpdateSnapshotReferencesOperation.java @@ -106,40 +106,41 @@ public UpdateSnapshotReferencesOperation replaceBranch(String name, long snapsho return this; } - public UpdateSnapshotReferencesOperation replaceBranch(String name, String source) { - return replaceBranch(name, source, false); + public UpdateSnapshotReferencesOperation replaceBranch(String from, String to) { + return replaceBranch(from, to, false); } - public UpdateSnapshotReferencesOperation fastForward(String name, String source) { - return replaceBranch(name, source, true); + public UpdateSnapshotReferencesOperation fastForward(String from, String to) { + return replaceBranch(from, to, true); } private UpdateSnapshotReferencesOperation replaceBranch( - String name, String source, boolean fastForward) { - Preconditions.checkNotNull(name, "Target branch cannot be null"); - Preconditions.checkNotNull(source, "Source ref cannot be null"); - SnapshotRef sourceRef = updatedRefs.get(source); - SnapshotRef refToUpdate = updatedRefs.get(name); - Preconditions.checkArgument(refToUpdate != null, "Target branch does not exist: %s", name); - Preconditions.checkArgument(sourceRef != null, "Ref does not exist: %s", source); - Preconditions.checkArgument(refToUpdate.isBranch(), "Ref %s is a tag not a branch", name); + String from, String to, boolean fastForward) { + Preconditions.checkNotNull(from, "Branch to update cannot be null"); + Preconditions.checkNotNull(to, "Destination ref cannot be null"); + SnapshotRef branchToUpdate = updatedRefs.get(from); + SnapshotRef toRef = updatedRefs.get(to); + Preconditions.checkArgument( + branchToUpdate != null, "Branch to update does not exist: %s", from); + Preconditions.checkArgument(toRef != null, "Ref does not exist: %s", to); + Preconditions.checkArgument(branchToUpdate.isBranch(), "Ref %s is a tag not a branch", from); // Nothing to replace - if (sourceRef.snapshotId() == refToUpdate.snapshotId()) { + if (toRef.snapshotId() == branchToUpdate.snapshotId()) { return this; } - SnapshotRef updatedRef = SnapshotRef.builderFrom(refToUpdate, sourceRef.snapshotId()).build(); + SnapshotRef updatedRef = SnapshotRef.builderFrom(branchToUpdate, toRef.snapshotId()).build(); if (fastForward) { boolean targetIsAncestor = SnapshotUtil.isAncestorOf( - sourceRef.snapshotId(), refToUpdate.snapshotId(), base::snapshot); + toRef.snapshotId(), branchToUpdate.snapshotId(), base::snapshot); Preconditions.checkArgument( - targetIsAncestor, "Cannot fast-forward: %s is not an ancestor of %s", name, source); + targetIsAncestor, "Cannot fast-forward: %s is not an ancestor of %s", from, to); } - updatedRefs.put(name, updatedRef); + updatedRefs.put(from, updatedRef); return this; } diff --git a/core/src/test/java/org/apache/iceberg/TestSnapshotManager.java b/core/src/test/java/org/apache/iceberg/TestSnapshotManager.java index d497dbd360a7..d561d697d3e9 100644 --- a/core/src/test/java/org/apache/iceberg/TestSnapshotManager.java +++ b/core/src/test/java/org/apache/iceberg/TestSnapshotManager.java @@ -409,15 +409,15 @@ public void testReplaceBranch() { } @Test - public void testReplaceBranchNonExistingTargetBranchFails() { + public void testReplaceBranchNonExistingBranchToUpdateFails() { Assertions.assertThatThrownBy( () -> table.manageSnapshots().replaceBranch("non-existing", "other-branch").commit()) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Target branch does not exist: non-existing"); + .hasMessage("Branch to update does not exist: non-existing"); } @Test - public void testReplaceBranchNonExistingSourceFails() { + public void testReplaceBranchNonExistingToBranchFails() { table.newAppend().appendFile(FILE_A).commit(); long snapshotId = table.currentSnapshot().snapshotId(); table.manageSnapshots().createBranch("branch1", snapshotId).commit(); @@ -427,6 +427,26 @@ public void testReplaceBranchNonExistingSourceFails() { .hasMessage("Ref does not exist: non-existing"); } + @Test + public void testFastForwardBranchNonExistingFromBranchFails() { + Assertions.assertThatThrownBy( + () -> + table.manageSnapshots().fastForwardBranch("non-existing", "other-branch").commit()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Branch to update does not exist: non-existing"); + } + + @Test + public void testFastForwardBranchNonExistingToFails() { + table.newAppend().appendFile(FILE_A).commit(); + long snapshotId = table.currentSnapshot().snapshotId(); + table.manageSnapshots().createBranch("branch1", snapshotId).commit(); + Assertions.assertThatThrownBy( + () -> table.manageSnapshots().fastForwardBranch("branch1", "non-existing").commit()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Ref does not exist: non-existing"); + } + @Test public void testFastForward() { table.newAppend().appendFile(FILE_A).commit(); @@ -445,7 +465,7 @@ public void testFastForward() { } @Test - public void testFastForwardWhenTargetIsNotAncestorFails() { + public void testFastForwardWhenFromIsNotAncestorFails() { table.newAppend().appendFile(FILE_A).commit(); table.newAppend().appendFile(FILE_B).set("wap.id", "123456789").stageOnly().commit();