-
Notifications
You must be signed in to change notification settings - Fork 116
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.
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
Implement "external" conflict resolution for merges #6631
Implement "external" conflict resolution for merges #6631
Conversation
407403a
to
05ca371
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass with some comments (incomplete)
...e/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogicImpl.java
Outdated
Show resolved
Hide resolved
...e/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogicImpl.java
Outdated
Show resolved
Hide resolved
...e/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java
Outdated
Show resolved
Hide resolved
...e/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java
Outdated
Show resolved
Hide resolved
...e/store/src/main/java/org/projectnessie/versioned/storage/versionstore/BaseCommitHelper.java
Show resolved
Hide resolved
...orage/common/src/main/java/org/projectnessie/versioned/storage/common/logic/CommitLogic.java
Outdated
Show resolved
Hide resolved
*/ | ||
@Nullable | ||
@jakarta.annotation.Nullable | ||
ObjId doCommit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI some of the changes here overlap with changes introduced in #6647, but so far they seem compatible.
05ca371
to
08041b8
Compare
08041b8
to
1e7d033
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple of comments on storing commit objects in case of conflicts (also relates to #6677)
if (mergeKeyBe.getExpectedTargetContent() == null) { | ||
keyDetailsMap.put(key, keyDetails(mergeBehavior, ConflictType.NONE)); | ||
return mergeBehavior == MergeBehavior.FORCE | ||
? ConflictResolution.IGNORE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe rename ConflictResolution.IGNORE
to ADD
for the sake of clarity? On the caller side it is not obvious that IGNORE
will cause the action to be added to the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do in a separate PR
|
||
// It's okay to do the fetchCommit() here and not complicate the surrounding logic (think: | ||
// local cache) | ||
StoreIndex<CommitOp> headIndex = indexesLogic(persist).buildCompleteIndexOrEmpty(head); | ||
verifyMergeTransplantCommitPolicies(headIndex, mergeCommit); | ||
|
||
mergeBehaviors.postValidate(); | ||
|
||
// TODO check keyDetailsMap for conflicts here - no need to persist the merge commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We should not call commitLogic.storeCommit()
if there are conflicts (same for transplanting individual commits).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do in a separate PR (same for the comment below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you wanna tackle it as part of #6677?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought so too :)
|
||
if (!indexesLogic.commitOperations(newCommit).iterator().hasNext()) { | ||
// No operations in this commit, skip it. | ||
continue; | ||
} | ||
|
||
CommitLogic commitLogic = commitLogic(persist); | ||
boolean committed = commitLogic.storeCommit(newCommit, emptyList()); | ||
boolean committed = commitLogic.storeCommit(newCommit, objsToStore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably not call this method if/when keyDetailsMap
contains conflicts.
|
||
// It's okay to do the fetchCommit() here and not complicate the surrounding logic (think: | ||
// local cache) | ||
StoreIndex<CommitOp> headIndex = indexesLogic(persist).buildCompleteIndexOrEmpty(head); | ||
verifyMergeTransplantCommitPolicies(headIndex, mergeCommit); | ||
|
||
mergeBehaviors.postValidate(); | ||
|
||
// TODO check keyDetailsMap for conflicts here - no need to persist the merge commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
Build on the API changes in #6619. Implements the logic for the new storage model.