[4.0.3 backport] CBG-5108: don't add backup rev loads to opposite lookup map that you have fetched by#8002
Conversation
…kup map that you have fetched by
There was a problem hiding this comment.
Pull request overview
This PR backports CBG-5108 to the 4.0.3 release branch, addressing an issue where backup revision loads incorrectly populate both lookup maps in the revision cache instead of only the map corresponding to the fetch method used (CV or RevID).
Changes:
- Modified revision cache logic to only populate the lookup map matching the fetch method (CV or RevID) when loading backup revisions
- Added test cases to verify correct behavior when fetching backup revisions and subsequently fetching current revisions
- Adjusted test expectations to account for potential duplicate cache entries from concurrent loads
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/replicatortest/replicator_test_legacy_rev_test.go | Skips a test pending fix from CBG-5106 |
| rest/blip_api_delta_sync_test.go | Adds test case for delta sync when delta is calculated from backup revision |
| db/revision_cache_test.go | Updates test backing store methods, adjusts memory calculations, modifies concurrent load assertions, removes obsolete test, and adds new test cases for backup revision loading behavior |
| db/revision_cache_lru.go | Simplifies post-load map population logic to prevent adding entries to opposite lookup map when loading backup revisions |
| db/revision_cache_interface.go | Adds nil checks for CV field and conditionally populates revID/CV only when fetching current revision |
| db/database_test.go | Adds test cases to verify correct behavior when fetching current revision after backup revision loads |
| db/blip_sync_context.go | Adds nil checks for CV field before using it in delta sync messages |
| legacyRev, _, err := collection.Put(ctx, docID, Body{"foo": "bar"}) | ||
| require.NoError(t, err) | ||
| revIDs = append(revIDs, legacyRev) | ||
| for range 2 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 2; i++ for better clarity and wider compatibility with older Go versions.
| legacyRev = newRev // OCC val | ||
| } | ||
| // simulate doc revs that are backed up to bucket pre upgrade | ||
| for i := range 2 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 2; i++ for better clarity and wider compatibility with older Go versions.
| db.FlushRevisionCacheForTest() | ||
|
|
||
| // fetch all three legacy revisions, first two should be loaded from old backup revisions | ||
| for i := range 3 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 3; i++ for better clarity and wider compatibility with older Go versions.
| _, ok := collection.revisionCache.Peek(ctx, docID, revIDs[2]) | ||
| require.True(t, ok) | ||
| // assert peek is false for revID1 and revID2 | ||
| for i := range 2 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 2; i++ for better clarity and wider compatibility with older Go versions.
| rev, doc1, err := collection.Put(ctx, docID, Body{"foo": "bar"}) | ||
| require.NoError(t, err) | ||
| docCVs = append(docCVs, doc1.HLV.GetCurrentVersionString()) | ||
| for range 2 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 2; i++ for better clarity and wider compatibility with older Go versions.
| rev = newRev // OCC val | ||
| } | ||
| // simulate doc revs that are backed up to bucket pre upgrade | ||
| for i := range 2 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 2; i++ for better clarity and wider compatibility with older Go versions.
| db.FlushRevisionCacheForTest() | ||
|
|
||
| // fetch all three legacy revisions, first two should be loaded from old backup revisions | ||
| for i := range 3 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 3; i++ for better clarity and wider compatibility with older Go versions.
| // at this point we have loaded 3 revs into a cache that can only hold 1 item | ||
| // assert that only 1 item exists in rev lookup (revID3 from above) | ||
| // to do so delete backup revs and fetch through rev cache assert first two have 404 errors whilst last one should succeed (resident in cache) | ||
| for i := range 2 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 2; i++ for better clarity and wider compatibility with older Go versions.
| err := collection.PurgeOldRevisionJSON(ctx, docID, revHash) | ||
| require.NoError(t, err) | ||
| } | ||
| for i := range 2 { |
There was a problem hiding this comment.
Using range with a numeric literal is valid Go syntax starting in Go 1.22, but it may not be familiar to all developers. Consider using a more explicit loop construct like for i := 0; i < 2; i++ for better clarity and wider compatibility with older Go versions.
2 participants
CBG-5108
clean cherry pick of a46b918
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/237/