CBG-4864 Import document if cv changes, even if body is unchanged #7775
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements CBG-4864 to ensure documents are imported when the current version (CV) changes, even if the document body hasn't changed. The change compares HLV version vector fields (_vv.ver and _vv.src) against sync metadata fields (_sync.rev.ver and _sync.rev.src) to determine if an import is needed.
Key changes:
- Enhanced
IsSGWritelogic to compare current version vectors with sync metadata - Added
CVOutdatedmethod to detect version mismatches - Updated import logic to handle CV changes separately from body changes
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 |
|---|---|
| db/document.go | Core logic for CV comparison and enhanced IsSGWrite methods |
| db/crud.go | Updated IsSGWrite calls to use new collection-level method |
| db/import.go | Modified import logic to use new needsImport method |
| db/import_listener.go | Updated feed-based import to use needsImport method |
| db/crud_test.go | Added comprehensive tests for IsSGWrite and CVOutdated functionality |
| db/util_testing.go | Added SafeDocumentName utility for test document naming |
| db/utilities_hlv_testing.go | Updated IsSGWrite call to use collection method |
| rest/blip_legacy_revid_test.go | Added removeHLV helper function and updated test calls |
| xdcr/xdcr_test.go | Updated test expectations due to behavior change |
adamcfraser
changed the title
db/document.go
Outdated
| if !isSGWrite || rawHLV == nil { | ||
| return isSGWrite, crc32Match | ||
| } | ||
| limitedHLV := struct { |
There was a problem hiding this comment.
Can we define 'limitedHLV' in hybrid_logical_version, and maybe a helper function there to just unmarshal cv?
db/document.go
Outdated
| return true, true, false | ||
| } | ||
| if !s.CVEqual(*extractedCV) { | ||
| return false, true, false |
There was a problem hiding this comment.
When this function returns crc32match=true, we increment the Crc32MatchCount stat. I think the intention of that stat is to track the number of documents that aren't imported because the body hasn't changed. Given that, I wonder if we should be returning false, false, false here.
There was a problem hiding this comment.
Ah, that's why the UserXattr comparison returns crc32 match = false even though the crc32 match was true. I thought that was a bug, but I didn't want to change for UserXattr comparison, but I'll change and add a comment here.
db/hybrid_logical_vector.go
Outdated
| return &currVersion | ||
| } | ||
|
|
||
| // ExtractCV is used to sastify CV only interface. Since it can never return an error, consider ExtractCurrentVersionFromHLV or GetCurrentVersion instead. |
There was a problem hiding this comment.
The comment here (about never returning an error) needs updating.
db/document.go
Outdated
| extractedCV, err := cv.ExtractCV() | ||
| if !errors.Is(err, base.ErrNotFound) { | ||
| if err != nil { | ||
| base.WarnfCtx(ctx, "Error extracting cv during IsSGWrite write check: %v", err) |
There was a problem hiding this comment.
I think info level logging here is sufficient, and we should remove the word 'error'. Maybe something like "Unable to extract cv during IsSGWrite check - skipping matching cv check: %v"
torcolvin
force-pushed
the
CBG-4864
branch
2 times, most recently
from
cd998b1 to
6cab66f
Compare
CBG-4864 make sure to import document if cv changes
Note: this causes behavior change in CBG-4878.
Notes for reviewer:
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/100/