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.

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

Support rebasing local changes on top of master/incoming changes #737

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

khanaffan
Copy link
Contributor

@khanaffan khanaffan commented Apr 23, 2024

Currently we Merge master changes into local changes of a briefcase when we do pullMerge. This method of change integration is very inflexible. It was decided way back we needed to move away from it to a more robust method that is more flexible and stable and works with editing workflows.

Change integration can be done using following two methods

  1. Merge (merge master on top of local changes)
  2. Rebase (merge local changes on top of master)

Note: No major merge happen as we use lock to prevent two user making conflicting changes to same row.

Sqlite allow both but its "Merge" does not work the same way as let's say git merge works. The whole method of merge is flawed in different ways as highlighted below. For example, there is not any merged changeset instead we have a rebase buffer that records decisions and then applies to the outgoing changeset which can cause some local changes to be lost altogether.

Merge

Merge goes as follows

  1. Download master changes.
  2. Apply it on top of local changes.
  3. Conflict resolved via conflict handler is captured in a rebase buffer.
  4. When creating an outgoing changeset rebase buffer is applied to the changeset which might result in some local changes made after the merge that might be lost unintentionally though they are part of local txns.
  5. In the case of the project extent story we were unable to push the union of two ranges mainly due to the merge ignoring local change when there was a conflict for that row when merging incoming changes. This lead to this PR as we cannot do anything meaningful with the current change integration strategy when we actively need to do some sort of merging or conflict resolution.

Pros

None that I know other than it can be faster as master changes are applied on top of local changes.

Cons

  1. Changeset on hub/master must be applied successfully to the briefcase. But in the case of "Merge" due to local conflicting changes, incoming changes might fail. This can leave the local briefcase in an inconsistent state where the user has no choice but to abandon their local briefcase. Merge cannot be reversed.
  2. There are no merge changeset as such. This means a merge as of now cannot be undone. So, it fails, it fails fatally.
  3. Merge is not very flexible if any conflict resolution wants to change or make new changes during the merge. As there is no active tracking, any changes during the merge will not be captured in merge txn.

Rebase

An alternate method to Merge is called Rebase. We do the following.

  1. Reverse all local changes
  2. Apply incoming master changes.
  3. Enable Tracking while reinstating reversed local changes.
    • All local changes are recomputed which means all old.* values in the local changeset are updated to tip.
    • Any new changes generated by the conflict handler are also captured as part of the local changeset.
    • User can still have undo history and can undo/redo them. Though local changes now have been updated with the latest changes from tip.
  4. Update local txn with new changes.

Pros

  1. All local changes are recomputed which means all old.* values in the local changeset are updated to tip.
    • Note here when reinstating the local txn, change tracking is enable so we recapture local txn when applied on top of master and update local txn.
  2. Undo history is preserved so application after pull/merge can still reverse or undo there local txn.
  3. Since we replay local txn we can also inform tool that generated the txn to see if need to update anything based on new changes.
  4. App can propagate/recompute changes between TxnManager::_onRebaseBeginLocalTxn()/TxnManager::_onRebaseBeginLocalTxn() when individual local txn are rebased. Any changes made during these are captured and local txn is updated.
  5. Conflict handler can also make new changes and will be captured and update local txn.
  6. When local change fail we still fail on local change.

Cons

Possible performance impact on pull/merge depending on size of local changes.

sequenceDiagram
    Actor User
    User->> iTwinjs: pullMerge()
    iTwinjs->> Hub: Download changesets
    iTwinjs->> iModelNative: beginPullMerge()
    iModelNative->> Briefcase: Reversse all local changes
    iModelNative ->> iTwinjs: beginPullMerge()
    iTwinjs->> iModelNative: Apply downloaded changesets
    iTwinjs->> iModelNative: endPullMerge()
    iModelNative ->> iModelNative: Begin tracking
    iModelNative ->> Briefcase: Re-apply local changes
    iModelNative ->> iModelNative: End tracking
    iModelNative ->> Briefcase: Update txn
Loading

As following shown by test their is certain conflict that will not happen in "Rebase".

DbConflictCause Rebase Merge Description
Data (Update) N/A Yes Two users update the same row.
Data (Delete) Yes Yes One user deletes and another updates it.
Conflict Yes Yes Two users insert a row with the same primary key.
NotFound N/A Yes The Parent row is deleted by one user and a child row for the same parent is inserted by another.
Constraint Yes Yes Unique constraint is violated.

itwinjs-core: iTwin/itwinjs-core#6662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant