Conversation
Redocly previews |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…backups - used to support delta sync and in-flight rev requests for 3.x clients
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for legacy RevTree IDs in Delta Sync by storing pointer documents that map RevTree IDs to their corresponding CV-based keys. This enables 3.x clients and delta sync operations to continue working with legacy RevTree ID format while the system internally uses CV-based revision storage.
Key changes include:
- Addition of a new database configuration option
store_legacy_revtree_data(renamed from delta sync-specific config) - Implementation of pointer documents that map RevTree IDs to CV hashes for backward compatibility
- Enhanced old revision retrieval logic to handle both direct revision bodies and pointer lookups
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/config.go | Adds new store_legacy_revtree_data boolean configuration field |
| rest/config_database.go | Sets default value for the new legacy RevTree data storage option |
| rest/server_context.go | Configures delta sync options with the new legacy revision storage setting |
| docs/api/components/schemas.yaml | Documents the new API configuration option in OpenAPI specs |
| db/database.go | Defines default value and adds field to DeltaSyncOptions struct |
| db/revision_old.go | Implements new pointer document functionality with non-JSON prefixes |
| db/revision.go | Refactors old revision retrieval to support pointer lookups and renames functions |
| db/crud.go | Updates delta sync logic and adds legacy RevTree pointer storage calls |
| db/revtree.go | Updates prefix checking to use new typed non-JSON prefix system |
| db/blip_handler.go | Removes unused parameter from GetDelta function call |
| rest/revision_old_revtree_test.go | Adds comprehensive test coverage for legacy RevTree ID retrieval |
| rest/utilities_testing_resttester.go | Updates test utilities to use template-based keyspace references |
| db/database_test.go | Adds test for delta sync with legacy RevTree IDs and updates existing tests |
| db/database_collection.go | Adds helper method to check legacy RevTree data storage setting |
| db/import.go | Updates function call to use renamed old revision storage method |
| base.DebugfCtx(ctx, base.KeyCRUD, "Found old revision pointer %q -> %q / %q", revOrCV, base.UD(docid), string(revData)) | ||
| return c.getOldRevisionJSON(ctx, docid, string(revData)) | ||
| default: | ||
| return nil, fmt.Errorf("unexpected prefix %b for old revision doc %q / %q", kind, base.UD(docid).String(), revOrCV) |
There was a problem hiding this comment.
Do you want to do, or are these equivalent?
| return nil, fmt.Errorf("unexpected prefix %b for old revision doc %q / %q", kind, base.UD(docid).String(), revOrCV) | |
| return nil, base.RedactErrorf("unexpected prefix %b for old revision doc %q / %q", kind, base.UD(docid).String(), revOrCV) |
There was a problem hiding this comment.
They're mostly equivalent here. The only difference that can occur when we use RedactErrorf is if we're using the error in a returned HTTP response, where the response gets the unredacted version, and the logging includes the redacted version.
In this case it can do that, but it's not a huge deal and I don't expect this default case to be hit anyway.
| default: 500 | ||
| store_legacy_revtree_data: | ||
| description: |- | ||
| Controls whether Sync Gateway stores additional legacy revision tree pointer data to support 3.x/early 4.x clients that still use RevTree IDs (for example when used as delta sources). |
There was a problem hiding this comment.
Is there any case other than delta sync where this is relevant?
I might phrase this in closer terms to functional impact. I'm not fussed on the particularly wording, take or leave this.
Controls whether Sync Gateway stores a mapping per document to map between a Sync Gateway < 4.0 revision ID and a Sync Gateway 3.0 revision ID.
This can be turned to false if delta_sync is disabled, or all clients (Couchbase Lite, Inter-Sync Gateway Replication, Sync Gateway) have documents that have been modified by Sync Gateway or Couchbase Lite 4.0+
If this only affects delta sync, should the parameter be tied to delta sync? Probably not, since you might lose the ability to enable delta sync after a document has been replicated.
There was a problem hiding this comment.
It was a config option in delta_sync originally but after discussing with Adam in Sync Up last week, this is also related to a feature where we store old revision bodies for 5 mins to cover old 2.x/3.x clients running replication underneath changing documents.
In those cases, the older clients don't have the replacement rev feature we added recently so they need to still be able to access the rev between the time the changes message sent and when it's actually replicated.
With that in mind, I moved the config option up and it can operate independently of delta_sync. It's also possible that store_legacy_revtree_data could be extended in the far future to actually rip out 3.x/RevTree support completely, but we're not quite there yet.
CBG-3748
Store a "pointer" revision backup doc keyed from a RevTree ID that points to the CV-based key for retrieval for 3.x clients and delta sync from RevTree IDs.
Can be disabled for customers that only have 4.x clients with a db-level config option, that can also control non-delta sync temporary revision backups (behind a TODO for 4.0, since it requires changes to restore previous functionality)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/61/