Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature]Implement replace branch in BranchManager #2911

Merged
merged 1 commit into from
May 28, 2024
Merged

[Feature]Implement replace branch in BranchManager #2911

merged 1 commit into from
May 28, 2024

Conversation

sunxiaojian
Copy link
Contributor

@sunxiaojian sunxiaojian commented Feb 27, 2024 •
edited
Loading

Purpose

Linked issue: close #2153

Tests

BranchActionITCase.testReplaceBranchToTargetBranch

API and Format

Documentation

@sunxiaojian
Copy link
Contributor Author

@FangYongs PTAL

@sunxiaojian
Copy link
Contributor Author

@FangYongs If you have time, could you please review this for me? thx

@FangYongs
Copy link
Contributor

@sunxiaojian Can you split the merge and replace branch in different PRs? I think the operations between them are quite different

@sunxiaojian
Copy link
Contributor Author

Can you split the merge and replace branch in different PRs? I think the operations between them are quite different

@FangYongs But they may use some of the same methods added in SnapshotManager and TagManager. The main methods for BranchManager are mergeBranch and replaceBranch.

@sunxiaojian sunxiaojian force-pushed the manage-branch-file branch 2 times, most recently from ced1e05 to 412a1ff Compare March 1, 2024 08:11
@TaoZex
Copy link
Contributor

TaoZex commented Mar 2, 2024 •
edited
Loading

#2862
Looks like there's been some conflict, do I need to close or change this pr?

@sunxiaojian
Copy link
Contributor Author

#2862 Looks like there's been some conflict, do I need to close or change this pr?

@TaoZex No need to close it. I will delete the merge branch section of this PR, and it will be based on yours.

@TaoZex
Copy link
Contributor

TaoZex commented Mar 3, 2024

#2862 Looks like there's been some conflict, do I need to close or change this pr?

@TaoZex No need to close it. I will delete the merge branch section of this PR, and it will be based on yours.

Thanks for your reply.

@sunxiaojian
Copy link
Contributor Author

sunxiaojian commented Mar 3, 2024 •
edited
Loading

@sunxiaojian Can you split the merge and replace branch in different PRs? I think the operations between them are quite different

@FangYongs The merge branch has been removed, and the implementation of the merge branch is mainly based on #2862

@sunxiaojian sunxiaojian changed the title [Feature]Implement merge/replace branch in BranchManager [Feature]Implement replace branch in BranchManager Mar 6, 2024
@schnappi17
Copy link
Contributor

@FangYongs Please help to review this pr also. And I suggest the merge/replace branches could wait for the Flink read/write pr merged and do detailed tests base on Flink reading/writing, because the merging and replacing branches will impact on the main branch, which is risky. WDYT?

@sunxiaojian sunxiaojian force-pushed the manage-branch-file branch 2 times, most recently from 30c6e01 to 06b4805 Compare March 7, 2024 12:18
@sunxiaojian sunxiaojian force-pushed the manage-branch-file branch 2 times, most recently from 0ed94a8 to 02421f0 Compare March 16, 2024 09:35
@sunxiaojian
Copy link
Contributor Author

@FangYongs @schnappi17 Is there anything else that needs to be modified?

@sunxiaojian sunxiaojian force-pushed the manage-branch-file branch 2 times, most recently from cbf7cbe to da40294 Compare April 24, 2024 11:47
@sunxiaojian sunxiaojian force-pushed the manage-branch-file branch 7 times, most recently from a16ddd7 to f803bd8 Compare May 27, 2024 04:12
@sunxiaojian sunxiaojian force-pushed the manage-branch-file branch 2 times, most recently from 248b5da to 8b39047 Compare May 27, 2024 09:10
@sunxiaojian sunxiaojian force-pushed the manage-branch-file branch from 8b39047 to ab21525 Compare May 27, 2024 09:24
@FangYongs
Copy link
Contributor

Thanks @sunxiaojian @schnappi17 , +1

@schnappi17
Copy link
Contributor

+1 for this, thanks @sunxiaojian @FangYongs

@schnappi17 schnappi17 merged commit 483a69c into apache:master May 28, 2024
9 checks passed
sunxiaojian added a commit to sunxiaojian/paimon that referenced this pull request May 28, 2024
@sunxiaojian
Implement replace branch in BranchManager (apache#2911)
5db7bfb
joyCurry30 pushed a commit to joyCurry30/paimon that referenced this pull request May 30, 2024
@sunxiaojian @joyCurry30
Implement replace branch in BranchManager (apache#2911)
bc495fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FangYongs FangYongs FangYongs left review comments

@schnappi17 schnappi17 schnappi17 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Implement merge/replace branch in BranchManager
4 participants
@sunxiaojian @FangYongs @TaoZex @schnappi17