Core, API: BaseReplacePartitions to branch test Refactoring #6650
Conversation
|
@jackye1995 can you also include this PR for the release ? This is just refactoring |
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Nonblocking comment, overall LGTM!
| TableMetadata base = TestTables.readMetadata(TABLE_NAME); | ||
| long baseId = | ||
| latestSnapshot(base, branch) == null ? -1 : latestSnapshot(base, branch).snapshotId(); | ||
| Snapshot latestSnapshot = latestSnapshot(base, branch); |
There was a problem hiding this comment.
We don't need to handle in this PR but given #6651 we will end up using the SnapshotUtil api which is introduced. We can either update this PR with the proposed utility API since there's consensus on that or just later refactor to use the utility API when it gets merged as part of branch writes. Since this is a test class, I'm okay either way.
There was a problem hiding this comment.
maybe we can just add the SnapshotUtil change in this PR? I don't think we can resolve the comments in that PR in time based on the current situation.
jackye1995
force-pushed
the
master_refactoring
branch
from
cd47f0d to
310f86c
Compare
|
@namrathamyske I updated based on Amogh's comment |
|
Merging since all comments are addressed, thanks for the review @yyanyy and @amogh-jahagirdar! |
4 participants
follow up from #5234, to refactoring minor comments left in last PR. @rdblue @amogh-jahagirdar please take a look at this.