Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBG-3026: Use DatabaseInitManager in all GSI cases #7121

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

bbrks
Copy link
Member

@bbrks bbrks commented Sep 20, 2024

CBG-3026

Rework database initialization code to reuse DatabaseInitManager in all GSI cases (including persistent config) to centralize and de-duplicate index creation code.\

Pre-req for CBG-2838 to rearrange per-collection/system:indexes iteration logic to minimise collection overhead when building/waiting for indexes.

Depends on

Integration Tests

Base automatically changed from CBG3026-prereq to main September 20, 2024 13:53
@bbrks bbrks marked this pull request as ready for review September 24, 2024 11:17
@bbrks bbrks self-assigned this Sep 24, 2024
@bbrks bbrks removed their assignment Sep 25, 2024
@bbrks bbrks requested review from torcolvin and adamcfraser and removed request for torcolvin September 25, 2024 17:45
Copy link
Collaborator

@torcolvin torcolvin left a comment

Choose a reason for hiding this comment

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

There are lots of comments because I was trying to resolve the complexity. I think that there's only one bug that I can spot which is closing the connection on ClusterN1QLStore if CouchbaseCluster.GetClusterN1QLStore is called when CouchbaseCluster.bucketConnectionMode == CachedClusterConnections.

I gave a lot of commentary on parts of this code I find confusing that might be worth fixing or not. I am OK if you decide not to implement any of the other ideas, but I think it's worth consdering adding code level comments for why they aren't implemented, because I will ask them next time I read this code. This could be a problem because I'm very familiar with this code that I'm asking these questions, and someone not familiar would not ask these questions.

rest/database_init_manager.go Outdated Show resolved Hide resolved
rest/server_context.go Show resolved Hide resolved
rest/database_init_manager.go Outdated Show resolved Hide resolved
rest/database_init_manager.go Show resolved Hide resolved
rest/server_context.go Show resolved Hide resolved
rest/server_context.go Show resolved Hide resolved
rest/server_context.go Show resolved Hide resolved
@torcolvin torcolvin assigned bbrks and unassigned torcolvin Sep 26, 2024
… (allows legacy configs with unique servers on each database to continue working). Had to be manually tested.
Copy link
Collaborator

@torcolvin torcolvin left a comment

Choose a reason for hiding this comment

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

I like the approach but I believe there are now bugs in the bootstrap connection.

Also, the thing that needs to be fixed from the original PR is whether to make ClusterOnlyN1QLStore.Close a no-op. That depends on whether CouchbaseCluster.GetClusterN1QLStore is called from a CouchbaseCluster that has CouchbaseCluster.bucketConnectionMode == PerUseClusterConnections (It should close, otherwise ClusterOnlyN1QLStore.Close needs to be a no-op

base/bootstrap.go Outdated Show resolved Hide resolved
rest/main.go Show resolved Hide resolved
rest/main.go Outdated Show resolved Hide resolved
rest/main.go Show resolved Hide resolved
rest/main.go Show resolved Hide resolved
…some cases, and replace with explicit connection in `InitializeDatabase`
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.

2 participants