-
Notifications
You must be signed in to change notification settings - Fork 13
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
Adding Rebase strategy to pull/merge #933
Open
khanaffan
wants to merge
14
commits into
main
Choose a base branch
from
affank/pull-merge-rebase-strategy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
khanaffan
requested review from
pmconne,
calebmshafer,
bbastings and
nick4598
as code owners
December 6, 2024 18:34
/azp run imodel-native |
Azure Pipelines successfully started running 1 pipeline(s). |
…m/iTwin/imodel-native into affanK/pull-merge-rebase-strategy
/azp run imodel-native |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run imodel-native |
Azure Pipelines successfully started running 1 pipeline(s). |
Please notify when builds are pssing. |
/azp run imodel-native |
Azure Pipelines successfully started running 1 pipeline(s). |
…fanK/pull-merge-rebase-strategy
its passing |
pmconne
approved these changes
Dec 16, 2024
…fanK/pull-merge-rebase-strategy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull merge & conflict resolution
What is change merging?
Change merging is when a briefcase pulls new changes from the iModel Hub and applies them locally. If the user has any local changes, the incoming changes need to be merged with the local changes. In this document, we are currently strictly talking about low-level SQLite changesets and high-level concepts defined in ECSchemas.
What is change conflicts
Git is used for text and text is two-dimensional data. Conflicts in two-dimensional data happen when two users change overlapping ranges of text. In a database, an overlapping change can be a change to the same row identified by a primary key. However, it could be more complex as that row might have database constraints like unique index, check, foreign key, triggers, etc., that make this row part of a larger chunk of information. SQLite tracks changes to individual tables/rows. It is up to the application to make changes consistently so as not to violate database constraints. If the database only contains a single table with no constraints, then it's basically the same as a text file as long as there is a primary key.
SQLite records changes by recording INSERT/UPDATE/DELETE operations with old and new values into a binary changeset. The order of changes in the changeset is arbitrary, but once the whole changeset is applied, it should bring the database to a new state that is consistent with all specified database constraints.
Things get interesting when there are local changes specifically to the same data as the incoming changeset. This operation is called pull-merge. Doing that can result in conflicts which from the SQLite perspective is as follows.
INSERT
Conflict
DELETE
NotFound
UPDATE
NotFound
Constraint
Data
Data
Constraint
ForeignKey
The foreign key one is the only one that is not associated with a specific row/table as it is a count of FK left unresolved and is a fatal error pointing to an error in application logic.
When SQLite applies a changeset and detects a conflict, it will call the conflict handler and request application feedback on how to resolve it. For the above, we can generally choose
SKIP
,REPLACE
, orABORT
. If we chooseREPLACE
, in some cases, our conflict handler may be called again if replacing the local row causes another conflict due to database constraints. We are looking at one row at a time, but these rows do not exist in isolation. They are connected to the rest of the rows in the same table via constraints, to the table definition via CHECK constraints, or to other tables via foreign keys. This makes change merging in the database a bit more complex.Types of change merging methods
Now that we have a somewhat understanding of key concepts about change merging and conflicts, we can talk about how to merge conflicting changes.
itwinjs supports two types of change merging:
Note: Since a briefcase as of now use locks to serialize changes between two or more users. The two methods specified does not really make any difference. The locks ensure only one user can change a given element and thus we do not expect low level sqlite conflict for any directly changed data.
"Merge" method
Merge is simple and straightforward but not flexible and can easily cause the creation of a changeset that might look fine at the local file level but fail when applied to a new file.
It is done as follows:
As we can see from the above, this merge method works fine if you lock individual rows in a table so only one user can change it. This workflow cannot really scale, and without a lock, it is completely useless.
"Rebase" method
This is a new method that has been implemented that is way more flexible. It is similar to git rebase. It is simple and easy to understand and will never lead to the push of a changeset that cannot be applied without locks.
SKIP
,REPLACE
, &ABORT
.As one of the most important take away in rebase is application control of merge. It has tools that made those changes and can correct redo them over master if needed. Where as "Merge" method is one way and does not give application any control over merge.
Rebase API workflow
At the moment only one set of changes does not require locks and we will use that as example. Its local properties of briefcase. Currently two user can change them and it can cause conflicts.
After downloading briefcase you can explicitly change merge method to rebase.
you can also set it by default this will cause any new briefcase to automatcally use "Rebase" as default method.
We download and open two briefcases for the same iModel, then set a property on b1 and push the changes. Note that the property being set did not exist, so it causes an INSERT operation at the database level.
Now b2 does the same and sets a different value, which also causes an INSERT operation being recorded.
Above threw exception as after undoing local changes then applying incoming changes we now have an entry for the key
test
in the db. While locally we recorded an insert operation for thetest
property. This causes a primary key violation. This action leaves the database in rebase mode.Next, we will install a conflict handler to resolve this issue. This conflict handler concatenates the master value with the local.
We resume rebase and it will resolve the conflict, allowing us to push the changes.
The final value of test pushed will be
foo + goo
.itwinjs-core: iTwin/itwinjs-core#7458