-
Notifications
You must be signed in to change notification settings - Fork 214
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 #7458
base: master
Are you sure you want to change the base?
Conversation
…ffank/pull-merge-rebase-strategy
…ffank/pull-merge-rebase-strategy
…ffank/pull-merge-rebase-strategy
…ffank/pull-merge-rebase-strategy
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.
This is a big exciting new feature, but nobody can currently use it. What needs to happen before they can?
I don't think this should be a public API at all, because it doesn't make any sense to have to choose between them. If there's a conflict, the only way out is to rebase. There doesn't need to be an option - just always do merge-then-rebaseOnFailure. That will be the most efficient anyway. BTW, you mentioned that you think rebasing eliminates the need for a schemaDb, but I don't understand how they're related at all. Maybe we should talk? |
/** @internal */ | ||
public readonly onRebaseTxnBegin = new BeEvent<(txn: TxnArgs) => void>(); | ||
/** @internal */ | ||
public readonly onRebaseLTxnEnd = new BeEvent<(txn: TxnArgs) => void>(); |
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.
public readonly onRebaseLTxnEnd = new BeEvent<(txn: TxnArgs) => void>(); | |
public readonly onRebaseTxnEnd = new BeEvent<(txn: TxnArgs) => void>(); |
core/backend/src/TxnManager.ts
Outdated
} | ||
|
||
if (args.cause === DbConflictCause.Constraint) { | ||
if (Logger.isEnabled(category, LogLevel.Info)) { |
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.
do we really need to wrap log statements to specific levels with checks if that level is enabled?
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.
Its copied to produce same errors/warning as in native. To match logs. But in case of rebase we do not need to be similar as its new code. I will update this method to minimally log and use different category.
…ffank/pull-merge-rebase-strategy
| | | 2. PRIMARY KEY does exist but other fields values does not match | `Data` | | ||
| | | 3. Database constraint voliation e.g. UNIQUE, CHECK caused by update | `Constraint` | | ||
| | `UPDATE` | 1. PRIMARY KEY does not exist | `NotFound` | | ||
| | | 2. PRIMARY KEY does not exist but data fields values does not match | `Data` | |
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.
I don't understand why this says PRIMARY KEY does not exist, did you mean does exist?
So I tested following locally that remove need to choose pullmerge method. FastForward: In this method we apply changeset on top of local changes but abort on any directly change conflict. Indirect changes conflict does not cause failure they are REPLACED. Schema changes cannot be applied using FastForward as they bound to cause conflict.
Above simplify things. App does not need to decide the merge method. Its all internal. |
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
Constraint
DELETE
NotFound
Data
Constraint
UPDATE
NotFound
Data
Constraint
ForeignKey
Above conflict can be resolved in on of the followed allowed ways. A
REPLACE
resolution may causeCONSTRAINT
conflict afterword if db constrain are voilated byREPLACE
action. IfCONSTRAINT
conflict is skipped then it can causeForeignKey
voliation at the end of changeset apply.INSERT
Conflict
ABORT
Constraint
SKIP
DELETE
NotFound
ABORT
Data
REPLACE
Constraint
SKIP
UPDATE
NotFound
ABORT
Data
REPLACE
Constraint
SKIP
ForeignKey
ABORT
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
.imodel-native: iTwin/imodel-native#933