CBG-3899 Write _mou on import, resync #6834

adamcfraser · 2024-05-17T22:21:29Z

Writes _mou on import and resync, populating cas and pCas. If an imported or resync'd document already has _mou where doc.cas=_mou.cas, updates _mou.cas but preserves existing pCas.

Stores MetadataOnlyUpdate struct in document but not SyncData, to avoid unmarshalling work when mou isn't needed.

CBG-3899

Integration Tests

GSI=true,xattrs=true 7.6.1 https://jenkins.sgwdev.com/job/SyncGateway-Integration/2445/
GSI=true,xattrs=true 7.2.0 https://jenkins.sgwdev.com/job/SyncGateway-Integration/2452/

Writes _mou on import and resync, populating cas and pCas. If an imported or resync'd document already has _mou where doc.cas=_mou.cas, updates _mou.cas but preserves existing pCas. Stores MetadataOnlyUpdate struct in document but not SyncData, to avoid unmarshalling work when mou isn't needed.

bbrks

LGTM - will let @torcolvin have a pass

torcolvin

Apologizes if I didn't reason correctly about on demand imports, but I thought it was worth testing at least.

torcolvin · 2024-05-20T22:32:51Z

db/background_mgr_resync_dcp_test.go

+		t.Skip("Test requires multi-xattr subdoc operations, CBS 7.6 or higher")
+	}
+
+	db.Options.QueryPaginationLimit = 100 // Required for principal ID query to not deadlock


I this from copypasta? I don't even see what would do query, so I imagine it might be from query based resync?

It's like the comment says - otherwise the principal ID query will deadlock when QueryPaginationLimit is zero, and we normally initialize this outside the db package. Possibly worth cleanup, but not in this PR I don't think.

torcolvin · 2024-05-20T22:37:24Z

db/background_mgr_resync_dcp_test.go

+	err = WaitForConditionWithOptions(t, func() bool {
+		var status BackgroundManagerStatus
+		rawStatus, _ := resyncMgr.GetStatus(ctx)
+		_ = json.Unmarshal(rawStatus, &status)
+		return status.State == BackgroundProcessStateCompleted
+	}, 200, 200)


just to make this easier to debug for the future, so it will print the status if it does fail:

Suggested change

err = WaitForConditionWithOptions(t, func() bool {

var status BackgroundManagerStatus

rawStatus, _ := resyncMgr.GetStatus(ctx)

_ = json.Unmarshal(rawStatus, &status)

return status.State == BackgroundProcessStateCompleted

}, 200, 200)

require.EventuallyWithT(t, func(c *assert.CollectT) {

var status BackgroundManagerStatus

rawStatus, err := resyncMgr.GetStatus(ctx)

assert.NoError(t, err)

assert.Equal(t, json.Unmarshal(rawStatus, &status)

assert.Equal(t, BackgroundProcessStateComplete, status.State)

}, 40 * time.Second, 200 * time.Millisecond)

torcolvin · 2024-05-20T22:40:02Z

db/background_mgr_resync_dcp_test.go

@@ -444,6 +445,93 @@ function sync(doc, oldDoc){
 	return db, ctx
 }

+// TestResyncMou ensures that resync updates create mou, and preserve pcas in mou in the case where resync is reprocessing an import


This would need a separate test and I'm not sure that this is common, but if you have resync running without auto import, this might be worth testing an inline {{_sync}} attribute.

torcolvin · 2024-05-20T22:42:36Z

db/change_cache.go

@@ -401,6 +401,7 @@ func (c *changeCache) DocChanged(event sgbucket.FeedEvent) {
 	}

 	// If using xattrs and this isn't an SG write, we shouldn't attempt to cache.
+	rawUserXattr := rawXattrs[collection.userXattrKey()]


nit:

I'd drop this assignment into this if statement since it isn't needed outside this block

It's used below (line 497).

torcolvin · 2024-05-20T22:46:57Z

db/import_test.go

+
+	// verify mou contents
+	if db.Bucket.IsSupported(sgbucket.BucketStoreFeatureMultiXattrSubdocOperations) {
+		mouXattr, ok := xattrs[base.MouXattrName]


Is it worth making sure this xattr is not present?

torcolvin · 2024-05-20T22:49:31Z

db/document.go

 	}
 	if err != nil {
 		return nil, err
 	}

+	if len(mouXattrData) > 0 {
+		if err := base.JSONUnmarshal(mouXattrData, &doc.metadataOnlyUpdate); err != nil {
+			if err != nil {


this is an extra err check here?

torcolvin · 2024-05-20T22:50:11Z

db/document.go

+	if len(mouXattrData) > 0 {
+		if err := base.JSONUnmarshal(mouXattrData, &doc.metadataOnlyUpdate); err != nil {
+			if err != nil {
+				base.WarnfCtx(ctx, "Failed to unmarshal mouXattr for key %v, will be ignored. Err: %v mou:%s", base.UD(docid), err, mouXattrData)


I'd clarify that only the _mou is ignored and not unmarshalling the document

torcolvin · 2024-05-20T22:50:24Z

db/document.go

 		// If the sync xattr is present, use that to build SyncData
-		if len(rawSyncXattr) > 0 {
+		if ok && len(rawSyncXattr) > 0 {


I don't see how this could be !ok and len(rawSyncXattr) > 0 , is there something I'm missing?

I think it was just my instinct to be more idiomatic and check the ok, but you're right that the length check is sufficient, and any performance difference is negligible.

torcolvin · 2024-05-20T22:54:36Z

db/import_test.go

@@ -24,6 +24,55 @@ import (
 	"github.com/stretchr/testify/assert"
 )

+func TestFeedImport(t *testing.T) {


I think this test should test for inline sync metadata, which I think should come with _mou, from the mou from after migrateMetadata.

I think without this that eventing will do a single echo, once after migrate metadata, and once after sync comes in.

My best guess as to how these occur in a real world is a backup/restore.

I also think that potentially there should be a test that only does on demand imports. This might not be a big deal, but we see on demand imports when there are rapid number of writes. It probably won't echo much since there will be an immediate write with different body immediately after.

Since migrate isn't a metadata-only update (it modifies the document body) and doesn't modify _mou, I didn't think additional test cases for specifically for migrate (inline sync metadata) were necessary.

I agree about the on-demand import case, I'll add that.

torcolvin · 2024-05-20T23:03:17Z

db/document.go

+// computeMetadataOnlyUpdate computes a new metadataOnlyUpdate based on the existing document's CAS and metadataOnlyUpdate
+func computeMetadataOnlyUpdate(currentCas uint64, currentMou *MetadataOnlyUpdate) *MetadataOnlyUpdate {
+	var prevCas string
+	currentCasString := string(base.Uint64CASToLittleEndianHex(currentCas))


Suggested change

currentCasString := string(base.Uint64CASToLittleEndianHex(currentCas))

currentCasString := base.CasToString(currentCas)

adamcfraser added 3 commits May 17, 2024 15:20

PR cleanup and resync fix

9cda405

Only enable mou when multi-xattr subdoc operations are available

88ee978

bbrks previously approved these changes May 20, 2024

View reviewed changes

torcolvin reviewed May 20, 2024

View reviewed changes

torcolvin assigned adamcfraser May 21, 2024

Address PR feedback, additional tests and handling for on-demand import

af76e20

adamcfraser dismissed bbrks’s stale review via af76e20 May 22, 2024 23:10

adamcfraser assigned torcolvin and unassigned adamcfraser May 22, 2024

Expect not found errors fetching mou for pre-7.6 tests

86dae36

torcolvin approved these changes May 23, 2024

View reviewed changes

adamcfraser merged commit 7359b30 into main May 23, 2024
34 checks passed

adamcfraser deleted the CBG-3899 branch May 23, 2024 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-3899 Write _mou on import, resync #6834

CBG-3899 Write _mou on import, resync #6834

adamcfraser commented May 17, 2024 •

edited

Loading

bbrks left a comment

torcolvin left a comment

torcolvin May 20, 2024

adamcfraser May 22, 2024

torcolvin May 20, 2024

torcolvin May 20, 2024

torcolvin May 20, 2024

adamcfraser May 22, 2024

torcolvin May 20, 2024

torcolvin May 20, 2024

torcolvin May 20, 2024

torcolvin May 20, 2024

adamcfraser May 22, 2024

torcolvin May 20, 2024

adamcfraser May 22, 2024

torcolvin May 20, 2024

	currentCasString := string(base.Uint64CASToLittleEndianHex(currentCas))
	currentCasString := base.CasToString(currentCas)

CBG-3899 Write _mou on import, resync #6834

CBG-3899 Write _mou on import, resync #6834

Conversation

adamcfraser commented May 17, 2024 • edited Loading

bbrks left a comment

Choose a reason for hiding this comment

torcolvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcfraser commented May 17, 2024 •

edited

Loading