CBG-5397: On-demand import should consider mou when updating HLV#8349
Conversation
There was a problem hiding this comment.
Pull request overview
Updates on-demand import read paths to ensure metadata-only update (MOU) state and rev-seq information are available when reloading documents prior to import, preventing incorrect HLV/MOU updates. Adds targeted test coverage to catch regressions around _mou and PreviousRevSeqNo population.
Changes:
- Update
GetDocumentWithRawreload to request_mouand$document.revidalongside existing SG xattrs. - Update
GetDocSyncDatato fetch$document.revid(and_mou) so on-demand import can populateMetadataOnlyUpdate.PreviousRevSeqNocorrectly. - Add tests validating
_mouis included in the reload path and thatGetDocSyncData-triggered import sets correct MOUpRev.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| db/import_test.go | Adds regression tests covering _mou presence during reload and correct PreviousRevSeqNo after on-demand import triggered by GetDocSyncData. |
| db/crud.go | Expands xattr keysets used during on-demand import-related reads to include _mou and $document.revid. |
| @@ -86,7 +86,7 @@ func (c *DatabaseCollection) GetDocumentWithRaw(ctx context.Context, docid strin | |||
| // If existing doc wasn't an SG Write, import the doc. | |||
| if !isSgWrite { | |||
| // reload to get revseqno for on-demand import | |||
| // rawBucketDoc.Xattrs reflects the pre-import state fetched at the reload step. | ||
| // Without the fix, _mou was absent from the reload xattr list, so this would be nil | ||
| // even though the bucket has a _mou xattr from the first import. |
adamcfraser
left a comment
adamcfraser
left a comment
There was a problem hiding this comment.
Have a look at the recommendation around when to fetch mou for the GetDocSyncData case.
| // Retrieve doc and xattr from bucket, unmarshal only xattr. | ||
| // Triggers on-demand import when document xattr doesn't match cas. | ||
| rawDoc, xattrs, cas, getErr := c.dataStore.GetWithXattrs(ctx, key, c.syncGlobalSyncAndUserXattrKeys()) | ||
| rawDoc, xattrs, cas, getErr := c.dataStore.GetWithXattrs(ctx, key, c.syncGlobalSyncMouRevSeqNoAndUserXattrKeys()) |
There was a problem hiding this comment.
In the ticket it's mentioned that we only want to do this additional work (to fetch mou, revSeqno) when we determine we need to do an on-demand import (i.e. after line 190). It's better to pay the larger performance overhead in the corner case (on-demand import), and avoid the incremental performance hit for the more common case (no import required).
- use mou and revSeq only for on demand import
RIT3shSapata
assigned
adamcfraser and
RIT3shSapata and unassigned
RIT3shSapata and
adamcfraser
| // Re-unmarshal with the full xattr set so doc.RevSeqNo and doc.MetadataOnlyUpdate | ||
| // are populated for use by OnDemandImportForGet. | ||
| doc, unmarshalErr = c.unmarshalDocumentWithXattrs(ctx, docid, nil, xattrs, cas, DocUnmarshalSync) | ||
| if unmarshalErr != nil { | ||
| return emptySyncData, unmarshalErr | ||
| } |
There was a problem hiding this comment.
another SG node cannot import it, so this comment is not really relevant
There was a problem hiding this comment.
@RIT3shSapata Another node could import it. We're doing an on-demand import here, another node could perform an import by either:
- the import feed
- a different on-demand import
It's correct that you need to re-check isSGWrite after the new fetch.
| // Re-unmarshal with the full xattr set so doc.RevSeqNo and doc.MetadataOnlyUpdate | ||
| // are populated for use by OnDemandImportForGet. | ||
| doc, unmarshalErr = c.unmarshalDocumentWithXattrs(ctx, docid, nil, xattrs, cas, DocUnmarshalSync) | ||
| if unmarshalErr != nil { | ||
| return emptySyncData, unmarshalErr | ||
| } |
There was a problem hiding this comment.
@RIT3shSapata Another node could import it. We're doing an on-demand import here, another node could perform an import by either:
- the import feed
- a different on-demand import
It's correct that you need to re-check isSGWrite after the new fetch.
- check if document is imported before performing on demand import
CBG-5397
Describe your PR here...
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests