[4.0.3 backport] CBG-5142: Only fetch backup revision for fromRev for delta sync #8048
Conversation
There was a problem hiding this comment.
Pull request overview
This PR backports CBG-5142 to version 4.0.3, fixing an issue with delta sync and backup revision fetching. The change ensures that backup revisions are only fetched for delta sync when necessary, specifically for the fromRev parameter.
Changes:
- Modified
GetWithCVmethod signature to accept aloadBackupparameter to control whether backup revisions should be loaded - Updated delta sync logic to only fetch backup revisions for the source revision (
fromRev), not the target revision - Updated all call sites of
GetWithCVto explicitly specify whether backup revision loading is needed
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| db/revision_cache_interface.go | Added loadBackup parameter to GetWithCV and getCurrentVersion methods |
| db/revision_cache_lru.go | Updated cache implementation to respect loadBackup flag for CV fetches |
| db/revision_cache_bypass.go | Updated bypass cache to use loadBackup parameter |
| db/crud.go | Updated delta sync to load backup revisions only for fromRev |
| db/utilities_hlv_testing.go | Added optional body parameter to InsertWithHLV helper method |
| db/revision_cache_test.go | Updated tests to use new GetWithCV signature and test backup revision loading |
| rest/api_test.go | Updated tests to verify backup revision fetching behavior through cache API |
| rest/attachment_test.go | Updated tests to handle new behavior where CV fetches without backup loading return not found |
| xdcr/xdcr_test.go | Updated test helper calls with new nil body parameter |
| rest/blip_api_crud_test.go | Updated test helper calls with new nil body parameter |
| rest/replicatortest/replicator_conflict_test.go | Updated cache calls with new loadBackup parameter |
| rest/blip_api_delta_sync_test.go | Added comprehensive tests for delta sync with bypass rev cache scenarios |
| db/hybrid_logical_vector_test.go | Updated test helper calls with new nil body parameter |
| db/database_test.go | Updated tests to use cache API directly for backup revision fetching |
| db/attachment_test.go | Updated test to use cache API directly for backup revision fetching |
|
|
||
| if err != nil { | ||
| rc.removeValue(value) // don't keep failed loads in the cache | ||
| rc.removeValue(value, collectionID) // don't keep failed loads in the cache |
There was a problem hiding this comment.
The collectionID parameter is passed to removeValue but not used in the function signature prior to this change. However, the function body uses fields from the value parameter to construct keys that should include collectionID. Verify that removeValue properly uses the collectionID parameter throughout the function body.
rest/attachment_test.go
Outdated
| attachments := body["_attachments"] | ||
| assert.Nil(t, attachments, "v1 should have no attachments") | ||
|
|
||
| if revType == "CV" && flushCache || revType == "CV" && base.TestDisableRevCache() { |
There was a problem hiding this comment.
The boolean logic should use parentheses for clarity. The current expression evaluates as (revType == \"CV\" && flushCache) || (revType == \"CV\" && base.TestDisableRevCache()), but explicit grouping would improve readability.
| if revType == "CV" && flushCache || revType == "CV" && base.TestDisableRevCache() { | |
| if revType == "CV" && (flushCache || base.TestDisableRevCache()) { |
| hlv = doc.HLV | ||
| } |
There was a problem hiding this comment.
The HLV assignment was moved inside the conditional block. However, this means that when the CV doesn't match the current version, hlv will remain nil. The previous code always assigned doc.HLV to hlv. This change could cause issues if code downstream expects hlv to contain the document's HLV even when fetching backup revisions.
| // create a few revs | ||
| docID := t.Name() | ||
| docCVs := make([]string, 0) | ||
| docCVs := make([]*Version, 0) |
There was a problem hiding this comment.
The variable name docCVs is now misleading as it stores *Version pointers rather than CV strings. Consider renaming to docVersions for clarity.
| require.Error(t, err) | ||
| require.ErrorContains(t, err, "missing") |
There was a problem hiding this comment.
When fetching by CV without loadBackup, the test expects an error. However, the test doesn't verify that backup revision loading works correctly for CV fetches. Consider adding an assertion that fetches with loadBackup=true succeeds after this failure.
CBG-5142
Backports 14787f7
Pre-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/261/