Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds HLV (Hybrid Logical Vector) conflict resolution support for inter-sync gateway replication (ISGR). It implements both local wins and remote wins conflict resolution strategies for version vector-based conflicts, along with comprehensive test coverage for various conflict scenarios including merge version handling and attachment preservation.
- Adds HLV conflict resolution functions and infrastructure to support vector-based conflict resolution
- Implements comprehensive test suite covering local wins, remote wins, merge version scenarios, and attachment handling
- Updates existing tests to support both legacy and HLV conflict resolution modes
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/replicatortest/replicator_test.go | Removes skips from tombstone tests and adds protocol version configuration |
| rest/replicatortest/replicator_conflict_test.go | Adds comprehensive HLV conflict resolution test suite with 1100+ lines of tests |
| rest/blip_api_crud_test.go | Removes skip from stat test |
| db/utilities_hlv_testing.go | Adds test helper function to alter document HLV for testing scenarios |
| db/util_testing.go | Updates function call to include conflict resolver parameter |
| db/sg_replicate_conflict_resolver.go | Adds HLV-aware conflict resolution functions and ResolveForHLV method |
| db/sg_replicate_cfg.go | Updates replicator config to support both legacy and HLV conflict resolvers |
| db/import_test.go | Updates function call to include conflict resolver parameter |
| db/hybrid_logical_vector_test.go | Adds unit tests for HLV conflict resolution algorithms |
| db/hybrid_logical_vector.go | Implements core HLV conflict resolution logic |
| db/crud_test.go | Updates function calls to include conflict resolver parameter |
| db/crud.go | Integrates HLV conflict resolution into document update flow |
| db/blip_handler.go | Updates BLIP handler to pass conflict resolver to HLV operations |
| db/active_replicator_config.go | Adds HLV conflict resolver function configuration |
|
|
||
| // Local doc (localDoc) is persisted in the bucket unlike the incoming remote doc (remoteDoc). | ||
| // Internal properties of the localDoc can be accessed from syc metadata. | ||
| // Internal properties of the localDoc can be accessed from sync metadata. |
There was a problem hiding this comment.
Typo in comment: "syc" should be "sync"
|
I plan to review this more, but I've created some helper functions #7704 to deal with updating HLV. These duplicate/obviate some of the code here. I needed to create these standalone functions to write unit tests that couchbase lite can use to ensure matching behavior. Those functions will be used for blip client testing and for ISGR. |
| } | ||
|
|
||
| // resolveLocalWinsHLV will update remote doc's body and attachments to match the local doc, and return a new HLV for local wins | ||
| func (db *DatabaseCollectionWithUser) resolveLocalWinsHLV(ctx context.Context, localDoc, remoteDoc *Document) (*HybridLogicalVector, error) { |
There was a problem hiding this comment.
There seem to be a few differences between resolveLocalWinsHLV and resolveDocLocalWins, some of which seem right, and some of which seem not right.
- There's not a new revtree id that is created in this function. You would need a local revtree id to support 3.x CBL replication even after 4.x ISGR replication.
- There's handling of branching tombstones. I'm not sure there will ever be branching since the local revtree is already overwritten by remote revtree by the time we are here? but I'm actually not sure.
- tombstoneActiveRevision means that the old revision is added to the doc.History for revtree ids.
There was a problem hiding this comment.
- New RevID is generated after this function is done inside PutExistsingCurrentVersion
2 & 3. There is work after this to reconcile revTree's on conflict resolution, the idea here is work for this will take place there
| LocalHLV *HybridLogicalVector | ||
| RemoteHLV *HybridLogicalVector |
There was a problem hiding this comment.
Are there going to be cases where only one HLV is present?
There was a problem hiding this comment.
Only both HLV's will be here, if one is missing it may be given an implicit HLV or we fall back to rev tree conflict handling.
| winningRev, ok := winner[BodyCV] | ||
| if !ok { | ||
| c.stats.ConflictResultMergeCount.Add(1) | ||
| return winner, ConflictResolutionMerge, nil | ||
| } |
There was a problem hiding this comment.
What are the cases where this gets hit vs the end of this function
There was a problem hiding this comment.
The aim here was to keep in sync with current existing conflict resolution functions.
Signed-off-by: Gregory Newman-Smith <gregory.newmansmith@couchbase.com>
torcolvin
left a comment
There was a problem hiding this comment.
LGTM, I think we can sort out the other stuff in a future PR.
| var newHLV *HybridLogicalVector | ||
| newHLV, err = db.resolveHLVConflict(ctx, doc, newDoc, conflictResolver.hlvConflictResolver) |
There was a problem hiding this comment.
nit
| var newHLV *HybridLogicalVector | |
| newHLV, err = db.resolveHLVConflict(ctx, doc, newDoc, conflictResolver.hlvConflictResolver) | |
| newHLV, err := db.resolveHLVConflict(ctx, doc, newDoc, conflictResolver.hlvConflictResolver) |
| remoteDoc.RemoveBody() | ||
| remoteDoc.Deleted = localDoc.IsDeleted() | ||
| remoteDoc.SetAttachments(localDoc.Attachments().ShallowCopy()) |
There was a problem hiding this comment.
I think this is fine for now, but is it possible that resolving an HLV conflict will also mean that affects a revtree as well?
I suspect it will need to call
Lines 1824 to 1862 in 02429a5
CBG-4778
AlterHLVForTestPre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/53/