Skip to content

API, Core: Fix naming in fastForwardBranch/replaceBranch APIs - #9134

Merged
amogh-jahagirdar merged 1 commit into
apache:mainfrom
amogh-jahagirdar:rename-source-target-ffapi
Nov 27, 2023
Merged

API, Core: Fix naming in fastForwardBranch/replaceBranch APIs#9134
amogh-jahagirdar merged 1 commit into
apache:mainfrom
amogh-jahagirdar:rename-source-target-ffapi

Conversation

@amogh-jahagirdar

@amogh-jahagirdaramogh-jahagirdar commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

This change fixes some very confusing naming from my original implementation of the fastForward/replaceBranch APIs.

Prior to this change, the APIs had the following naming:

fastForward(String name, String source)
replaceBranch(String name, String source)

"Name" is the branch that would be fast forwarded and source is the "destination" or target we would fast forward to. This doesn't really align with how most people would think when it comes to fast forward or replace operations (like Git).

This change renames it to:

fastForward(String from, String to)
replaceBranch(String from, String to)

Note: This is just a rename of parameters and error messages. There is no change in behavior in the API.

@amogh-jahagirdar

amogh-jahagirdar commented Nov 22, 2023

Copy link
Copy Markdown
ContributorAuthor

cc @rakesh-das08@singhpk234 This stems from our discussion on #8854 (comment)

Comment threadcore/src/test/java/org/apache/iceberg/TestSnapshotManager.java Outdated
Comment threadapi/src/main/java/org/apache/iceberg/ManageSnapshots.java Outdated
@amogh-jahagirdar
amogh-jahagirdarforce-pushed the rename-source-target-ffapi branch 9 times, most recently from d33430d to 08e41ceCompareNovember 23, 2023 01:59
Comment threadcore/src/test/java/org/apache/iceberg/TestSnapshotManager.java Outdated
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");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're a little inconsistent here in terms of null checking (throwing NPE vs IAE) but I guess that's out-of-scope for this PR, so just wanted to mention that

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, and this unfortunately applies to more than just replace/fastForward. (e.g. create, rename, as well throw an NPE instead of IAE). I'll take this in a follow on.

@amogh-jahagirdar

Copy link
Copy Markdown
ContributorAuthor

Merging, thanks for the reviews @nastra@rakesh-das08

@amogh-jahagirdar
amogh-jahagirdar merged commit 6fc5be7 into apache:mainNov 27, 2023
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@amogh-jahagirdar@nastra@rakesh-das08