Add rename_branch to ManageSnapshots - #3426
Conversation
abnobdoss
left a comment
abnobdoss
left a comment
There was a problem hiding this comment.
Looks good! Left two minor comments.
| if new_name in refs: | ||
| raise ValueError(f"Ref {new_name} already exists") | ||
|
|
||
| self.create_branch(ref.snapshot_id, new_name, ref.max_ref_age_ms, ref.max_snapshot_age_ms, ref.min_snapshots_to_keep) |
There was a problem hiding this comment.
How do we ensure refs is still the latest metadata when creating the new branch?
There was a problem hiding this comment.
Would it be possible to add some unit tests validating that there's no concurrency issues from commits on the branch while this operation runs?
There was a problem hiding this comment.
What does upstream do here for validation?
| if name == MAIN_BRANCH: | ||
| raise ValueError("Cannot rename main branch") | ||
|
|
||
| refs = self._transaction.table_metadata.refs | ||
| if name not in refs: | ||
| raise ValueError(f"Branch does not exist: {name}") | ||
|
|
||
| ref = refs[name] | ||
| if ref.snapshot_ref_type != SnapshotRefType.BRANCH: | ||
| raise ValueError(f"Ref {name} is not a branch") | ||
|
|
||
| if new_name in refs: | ||
| raise ValueError(f"Ref {new_name} already exists") |
There was a problem hiding this comment.
I think using stock ValueError makes sense here, looking at upstream preconditions is used!
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
3 participants
Rationale for this change
Iceberg Java offers renameBranch method.
This PR enhances the coverage of branch operations.
Are these changes tested?
Yes
Are there any user-facing changes?
Add
rename_branchtoManageSnapshots.