Skip to content

CBG-4244 cbgt: only recreate indexes if changed - #8467

Merged
adamcfraser merged 4 commits into
mainfrom
CBG-4244
Aug 6, 2026
Merged

CBG-4244 cbgt: only recreate indexes if changed#8467
adamcfraser merged 4 commits into
mainfrom
CBG-4244

Conversation

@torcolvin

@torcolvin torcolvin commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Do not call cbgt.Manager.CreateIndex if an index in the bucket already exists.

In a multi node Sync Gateway cluster running import:

  1. Create node 1
  2. Node 1 is assigned all 16 import partitions
  3. Node 2 joins
  4. Partitions are split in two sets of 8 on each node

Prior to this code review, the partitions that stayed on node 1 would have their feeds closed and recreated. In this code change, the definitions are not updated if they match what is in the bucket.

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

  • Link upstream PRs
  • Update Go module dependencies when merged

Integration Tests

Do not call cbgt.Manager.CreateIndex if an index in the bucket already exists.
Copilot AI review requested due to automatic review settings July 20, 2026 20:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves cbgt sharded-DCP index creation behavior so that Sync Gateway nodes do not redundantly “update” (and thereby rotate UUIDs / restart PIndexes) when the persisted index definition is already identical—preventing unnecessary DCP feed teardown/recreate during cluster topology changes (e.g., nodes joining during import).

Changes:

  • Add an “already up to date” check in cbgt index creation to skip no-op updates that would otherwise rotate the index UUID and restart PIndexes.
  • Introduce semantic equality helpers for sharded-DCP feed source/index params (order-insensitive / canonicalized JSON comparisons) and unit tests for them.
  • Add multi-node rebalance tests and new RestTesterCluster capabilities to add nodes and assert PIndex stability across rebalances.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
rest/utilities_testing_rest_tester_cluster.go Adds locking around cluster node list and introduces AddNode to simulate nodes joining an existing cluster.
rest/cbgt_rebalance_test.go New rebalance tests ensuring only moved PIndexes restart and unmoved PIndexes retain UUIDs.
db/util_testing.go Adds test helper types/functions to snapshot import PIndex state (UUID + source partitions).
base/dcp_test.go Adds tests ensuring cbgt index creation is idempotent and tolerant of transient metadata read errors; documents a known update/delete race.
base/dcp_sharded.go Implements “skip redundant update” behavior by comparing persisted IndexDef to the intended one before calling CreateIndex.
base/dcp_sharded_test.go Adds unit test coverage for cbgtIndexDefUnchanged behavior.
base/dcp_feed_type.go Adds equality helpers for feed params (canonical JSON + order-insensitive collections) for robust comparisons.
base/dcp_feed_type_test.go New unit tests covering SGFeedSourceParamsEqual / SGFeedIndexParamsEqual and collection order-insensitivity.

Comment thread rest/utilities_testing_rest_tester_cluster.go Outdated
Comment thread db/util_testing.go
Comment thread base/dcp_sharded.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@adamcfraser adamcfraser left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good approach, a couple of questions on the specifics.

Also, do we need to create some E2E tests to verify the functionality when not sharing cbgt/SGW globals?

Comment thread base/dcp_sharded.go
existing.SourceUUID == sourceUUID &&
existing.PlanParams.MaxPartitionsPerPIndex == planParams.MaxPartitionsPerPIndex &&
existing.PlanParams.NumReplicas == planParams.NumReplicas &&
SGFeedSourceParamsEqual(existing.SourceParams, sourceParams) &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works if cbgt will always persist SourceParams / IndexParams exactly as we send them (i.e. they don't do things like set default values, etc). Just to be sure, have you verified this is a safe assumption?

Comment thread base/dcp_sharded.go Outdated
if previousIndexUUID != "" && existingDef != nil &&
cbgtIndexDefUnchanged(existingDef, sourceType, c.sourceName, c.sourceUUID, sourceParams, opts.IndexType, indexParams, planParams) {
InfofCtx(ctx, KeyDCP, "cbgt index %q for db %q is already up to date, skipping redundant update", MD(indexName), MD(opts.DBName))
c.Manager.Kick("NewIndexesCreated")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this kick is for a race where the index isn't present when started the manager, but was created in between? I thought the manager would be listening for this in the usual way (i.e. the same way it detects indexes created on other nodes today). I'm fine if this is purely defensive ("it won't hurt to kick"), just want to understand the motivation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, it is defensive and I think I would rather not run an additional kick, reducing cbgt processing is part of the goal. Manager.Start has already run the appropriate Kick.

@adamcfraser adamcfraser assigned torcolvin and unassigned adamcfraser Jul 21, 2026
@bbrks bbrks assigned RIT3shSapata and unassigned torcolvin Jul 31, 2026
@torcolvin torcolvin assigned torcolvin and unassigned RIT3shSapata Aug 5, 2026
@torcolvin
torcolvin requested a review from a team August 5, 2026 20:34
@torcolvin torcolvin assigned adamcfraser and unassigned torcolvin Aug 5, 2026
@adamcfraser
adamcfraser merged commit 42cadd7 into main Aug 6, 2026
53 checks passed
@adamcfraser
adamcfraser deleted the CBG-4244 branch August 6, 2026 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants