Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Sync Gateway’s _sync:syncInfo document encoding and write path to support a rolling-upgrade safe transition to a binary-prefixed (V1) format once the cluster-wide minimum Sync Gateway version reaches 4.1+. It also threads cluster compatibility version resolution through DatabaseContextOptions so long-lived components observe changes during upgrades.
Changes:
- Introduces a versioned syncInfo encoding (legacy bare JSON vs. V1 discriminator byte + JSON) with
DecodeSyncInfohandling both. - Switches syncInfo writes from
Update/Addto aGetRaw + WriteCasCAS-retry loop to correctly set KV datatype (JSON vs binary) based on payload. - Plumbs cluster compat version resolution into DB context options and key call sites (bootstrap, resync DCP, attachment migration), with tests covering upgrade gating and both encodings.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
rest/server_context.go |
Passes cluster compat version into syncInfo init and stores a closure on DB context options for dynamic re-resolution during rolling upgrades. |
rest/config_manager.go |
Switches syncInfo read during metadataID computation to raw fetch + DecodeSyncInfo to support V1. |
rest/config_manager_test.go |
Adds coverage for binary/V1 syncInfo decoding in computeMetadataID and corrupt-doc fallback behavior. |
rest/cluster_compat_test.go |
Adds a rolling-upgrade gate test ensuring syncInfo stays legacy with a 4.0 peer and flips to V1 at 4.1+. |
db/util_testing.go |
Updates syncInfo meta version assertion helper to use raw fetch + DecodeSyncInfo. |
db/database.go |
Adds ClusterCompatVersion func() *base.ClusterCompatVersion to DatabaseContextOptions. |
db/database_test.go |
Updates tests for new syncInfo function signatures (passing cluster compat version). |
db/background_mgr_resync_dcp.go |
Passes resolved cluster compat version into syncInfo update after resync completion. |
db/background_mgr_resync_dcp_test.go |
Adds end-to-end test verifying resync writes V1 syncInfo when ccv>=4.1. |
db/background_mgr_attachment_migration.go |
Passes resolved cluster compat version into syncInfo meta version update after migration. |
db/background_mgr_attachment_migration_test.go |
Adds end-to-end test verifying attachment migration writes V1 syncInfo when ccv>=4.1. |
base/rosmar_cluster.go |
Fixes GetDocument exists return semantics and adds GetRawDocument implementation for bootstrap reads. |
base/leaky_datastore.go |
Adds WriteCasCallback injection seam for CAS write path testing. |
base/leaky_bucket.go |
Adds WriteCasCallback config option to support new syncInfo write path testing. |
base/constants_syncdocs.go |
Implements versioned syncInfo encoding/decoding and CAS-based write/update logic using raw bytes + datatype control. |
base/constants_syncdocs_test.go |
Updates error-injection tests for new write path and adds comprehensive tests for V1/legacy encodings and decode behavior. |
base/bootstrap.go |
Extends BootstrapConnection with GetRawDocument and adds Couchbase Server implementation using raw transcoder. |
base/bootstrap_test.go |
Adds tests for GetRawDocument and for correct GetDocument exists behavior across Rosmar/Couchbase implementations. |
| type syncInfoMetaVersion byte | ||
|
|
||
| const ( | ||
| // SyncInfoTypeUnknown is an unused byte value but here for clarity between the zero value | ||
| SyncInfoTypeUnknown syncInfoMetaVersion = iota | ||
| // SyncInfoTypeV1 is used to denote a sync info document in version 4.1 and later | ||
| SyncInfoTypeV1 | ||
| ) |
There was a problem hiding this comment.
Changed to syncInfoEncodingType
| var ccv *base.ClusterCompatVersion | ||
| if sc.ClusterCompat != nil { | ||
| ccv = sc.ClusterCompat.ClusterCompatVersion() | ||
| } |
There was a problem hiding this comment.
This pattern feels like there should just be a function on ServerContext that returns cluster compat version or nil and is doing all of its own nil checking, etc. inside.
| ImportVersion uint64 // Version included in import DCP checkpoints, incremented when collections added to db | ||
| DisablePublicAllDocs bool // Disable public access to the _all_docs endpoint for this database | ||
| StoreLegacyRevTreeData *bool // Whether to store additional data for legacy rev tree support in delta sync and replication backup revs | ||
| ClusterCompatVersion func() *base.ClusterCompatVersion // ClusterCompatVersion returns the current cluster-wide minimum SG version, or nil if not yet known. |
There was a problem hiding this comment.
CCV (or the function to get it) is better suited to be stored on DatabaseContext than in the options.
| var ccv *base.ClusterCompatVersion | ||
| if a.databaseCtx.Options.ClusterCompatVersion != nil { | ||
| ccv = a.databaseCtx.Options.ClusterCompatVersion() | ||
| } | ||
| if err := base.SetSyncInfoMetaVersion(ctx, dbc.dataStore, MetaVersionValue, ccv); err != nil { |
There was a problem hiding this comment.
likewise helper function on DatabaseContext that returns ccv or nil and does its own nil checking will help keep code dependent on CCV cleaner.
| if err != nil { | ||
| return nil, fmt.Errorf("Error marshalling syncInfo: %v", err) | ||
| } | ||
| if clusterCompatVersion != nil && clusterCompatVersion.AtLeast(4, 1) { |
There was a problem hiding this comment.
Could maybe improve the ergonomics of this API by pushing the nil check down into the AtLeast method itself (returning false is fine for that case)
A typed nil won't panic if the method is defined on the pointer and lets us avoid nil checks at usage time.
CBG-5366
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests