CBG-4790: reconcile rev tree on write for ISGR by gregns1 · Pull Request #7681 · couchbase/sync_gateway

gregns1 · 2025-08-13T16:37:16Z

PR to take the new revTree property in ISGR and includes those revisions in the write
There are some test cases that I would still like to write but require conflict resolution of some sort to be in place
Changes to tests that were temporarily asserting on CV only due to missing reconciliation of revTree. These tests now assert on CV and revID.

Updated some info on ISGR changes thus far in this document: https://docs.google.com/document/d/1SrN_om0pBm3yGnZF1cenFrkODhwMYWfPkjPbTq_2cEE/edit?usp=sharing

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

Link upstream PRs
Update Go module dependencies when merged

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/32/ (test flake)

Copilot

Pull Request Overview

This PR implements reconciliation of revision trees on write for Inter Sync Gateway Replication (ISGR) by taking the new revTree property and including those revisions in the write process. The changes ensure that revision IDs and versions are correctly synchronized between active and passive replicators.

Key changes:

Updated tests to assert on both CV and RevID instead of just CV due to fixed reconciliation
Added new revision tree reconciliation logic in CRUD operations
Extended BLIP handlers to process revision tree properties from ISGR

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
rest/replicatortest/replicator_test.go	Updated test assertions to check both CV and RevID, removed temporary workarounds for CBG-4790/4791
rest/replicatortest/replicator_revtree_test.go	Added comprehensive test cases for revision tree reconciliation in both pull and push scenarios
db/utilities_hlv_testing.go	Removed outdated comment about CBG-4790/4791 being unimplemented
db/util_testing.go	Updated function call to include new revTreeISGRProperty parameter
db/import_test.go	Updated function call to include new revTreeISGRProperty parameter
db/crud_test.go	Updated function calls to include new revTreeISGRProperty parameter
db/crud.go	Implemented core revision tree reconciliation logic with new parameter handling
db/blip_sync_context.go	Enhanced known revs processing to handle CV extraction and legacy document detection
db/blip_handler.go	Updated revision processing to use revision tree property for ISGR clients

torcolvin · 2025-08-14T11:05:41Z

 						if versionVectorProtocol {
-							legacyRev = true
+							if !cvInKnownRevs {
+								// if cv wasn't the first element in known revs array, this will be legacy doc


This should probably never happen, but this comment isn't correct:

If you had known revs as:

["1@abc", "1-abc", "2@def"]

this would end up incorrect. I find this logic incredibly hard to follow and I was thinking whether we could split this into a function with some table driven inputs.

I'm thinking something like:

getKnownRevs(changeRequestRow []any) (deltaSrcRevID string, knownRevs map[string]bool)

Then we could list valid conditions for

3.x replications

4.x replications

mixed?

I am having trouble holding in my head what the expected input and output is, and having a test for this can serve as a type of documentation.

Moved to function and test runs on that function.

torcolvin · 2025-08-14T11:20:16Z

 }

-func (db *DatabaseCollectionWithUser) PutExistingCurrentVersion(ctx context.Context, newDoc *Document, newDocHLV *HybridLogicalVector, existingDoc *sgbucket.BucketDocument, revTreeHistory []string) (doc *Document, cv *Version, newRevID string, err error) {
+func (db *DatabaseCollectionWithUser) PutExistingCurrentVersion(ctx context.Context, newDoc *Document, newDocHLV *HybridLogicalVector, existingDoc *sgbucket.BucketDocument, revTreeHistory []string, revTreeISGRProperty bool) (doc *Document, cv *Version, newRevID string, err error) {


I think you should describe the arguments to this function in a docstring.

I'm trying to wrap my head around why you'd have revTreeHistory but revTreeISGRProperty is false. This could be done in some type of test as well.

One way to make this more clear (not sure) is to have something that is:

type putRevTreeHistory struct { history []string // is this always going be ["1-abc", "2-abc"] forceAlignment bool // do not check for revtree conflicts? }

I don't like the above because I'm in fact not sure what this does.

My very shaky understanding is:

revTreeHistory could contain the incoming cv

revTreeHistory could contain the revtree from the incoming source? which would be the previous revisions? Could this be a 2@abc, 1-def?

Do we think that we should clarify with some type of typed information to make the subsequent calls easier to understand?

I really don't think this should mention ISGR at all, that's ultimately an implementation detail upstream. The boolean revTreeISGRProperty controls:

If true, generate a new revtree id rather than putting the existing revtree id

If true, will take all of this revtree history and add it to the existing revtree? or overwrite the history if there are conflicts?

avoid rev tree history conflict checking?

Because this behavior is connected to revHistory that's why I thought they might belong in the same struct.

Changed the naming and added comment to describe input parameters to the function.

I will be documenting legacy rev handling better soon too for knowledge of the team given this area can get very confusing in the code IMO.

torcolvin · 2025-08-14T11:27:07Z

 		// to add any new revision to rev tree below.
 		// Only check for rev tree conflicts if we haven't already checked above
-		if !revTreeConflictChecked && len(revTreeHistory) > 0 {
+		if !revTreeConflictChecked && len(revTreeHistory) > 0 && !revTreeISGRProperty {


Should we declare revTreeConflictChecked true if revTreeISGRProperty is set above? If not, I think that we should add to comment above why this isn't checked

Added comment here for clarity, will be updating the doc in the description with more legacy rev handling info soon too for team knowledge.

torcolvin · 2025-08-14T11:44:32Z

+	require.EventuallyWithT(t, func(c *assert.CollectT) {
+		assert.Equal(c, db.ReplicationStateStopped, ar.GetStatus(ctx1).Status)
+	}, time.Second*20, time.Millisecond*100)


For readability, I would be inclined to use:

rt1.WaitForReplicationStatus(replicationID, db.ReplicationStateStopped)

you might need to use RestTester.CreateReplication

I want to be using the active replicator object here not the rest api for these so will have to leave these assertions as is.

Its on roadmap to have these active replicator tests running like blip tester client does in two modes, it may not make sense to have these running in two modes given they test 4.0 features but I would still like that option.

torcolvin · 2025-08-14T11:48:18Z

+	assert.ElementsMatch(t, slices.Collect(maps.Keys(rt1Doc.History)), docHistoryList)
+}
+
+func TestActiveReplicatorPushRevTreeReconciliation(t *testing.T) {


Is this just the same test as the pull test? should these be parameterized? If this makes it too complicated I don't want to do that, but also if it really is mostly the same code then it might be more helpful?

The same is true for large size tests. If it doesn't make sense to use push and pull, does it make sense to parameterize doc numbers for the test?

Parameterized these tests

torcolvin · 2025-08-14T11:50:24Z

 	}
 }

+func (doc *Document) alignRevTreeHistory(ctx context.Context, newDoc *Document, revTreeHistory []string) error {


Add docstring and table test for this function with the expectations.

My understanding is that align might actually mean merge and overwrite if required, but it would be easier to see if we can have concrete examples.

Done and added test

torcolvin

LGTM, some logging suggestions

torcolvin · 2025-08-18T00:04:11Z

+				// extract cv from the known revs array
+				msgHLV, _, err := extractHLVFromBlipString(revID)
+				if err != nil {
+					base.DebugfCtx(ctx, base.KeySync, "Invalid known rev format for hlv on doc: %s falling back to full body replication.", base.UD(docID))


we are swallowing the error here, so we don't have much chance of ever debugging this

I don't think we want to error here fully given it can fall back to full body replication. Its possible that the first elem may be a revID (if the change response for this doc is a legacy rev) in this case we wouldn't want to error but maybe log to ensure its clear why its not being sent as a delta.

I have added the error message and the revID string to the log line though so we can see why we fall into this.

torcolvin · 2025-08-18T00:10:07Z

+	if doc.HLV == nil {
+		// no hlv on local doc, mark as missing but send current rev as known rev (will be handled as legacy
+		// rev document on changes response handler)
+		base.DebugfCtx(ctx, base.KeyChanges, "Doc %s has no HLV, marking change version %s as missing", base.UD(docid), base.UD(rev))


Do you think debug is too verbose for this? This seems common. If this is useful for testing, maybe KeyVV?

Good point, i'll drop to trace

gregns1 self-assigned this Aug 13, 2025

Copilot AI review requested due to automatic review settings August 13, 2025 16:37

Copilot AI reviewed Aug 13, 2025

View reviewed changes

Comment thread rest/replicatortest/replicator_revtree_test.go Outdated

gregns1 force-pushed the CBG-4790 branch from fbace24 to 6b12ce2 Compare August 14, 2025 09:02

gregns1 requested review from bbrks and torcolvin August 14, 2025 09:23

gregns1 assigned torcolvin and bbrks and unassigned gregns1 Aug 14, 2025

torcolvin reviewed Aug 14, 2025

View reviewed changes

torcolvin assigned gregns1 and unassigned torcolvin and bbrks Aug 14, 2025

gregns1 added 3 commits August 15, 2025 09:17

CBG-4790: reconcile rev tree on write for ISGR

682bccf

fix context

ed9a347

address commnets

cf13131

gregns1 force-pushed the CBG-4790 branch from aed75bf to cf13131 Compare August 15, 2025 08:17

gregns1 added 3 commits August 15, 2025 09:23

lint issue

bf8fedb

test issue

c291e77

update test to point to right ticket

9651271

gregns1 assigned torcolvin and unassigned gregns1 Aug 15, 2025

torcolvin reviewed Aug 18, 2025

View reviewed changes

updates to logging

df23c57

torcolvin approved these changes Aug 18, 2025

View reviewed changes

gregns1 merged commit 8d15a23 into main Aug 18, 2025
42 checks passed

gregns1 deleted the CBG-4790 branch August 18, 2025 14:17

Conversation

gregns1 commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-review checklist

Dependencies (if applicable)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gregns1 commented Aug 13, 2025 •

edited

Loading