-
Notifications
You must be signed in to change notification settings - Fork 140
CBG-4548 allow creation of partitioned indexes #7428
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
Conversation
bbrks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this PR was intended to implement custom partition counts other than 8. The ticket and PR were a bit vague in terms of just stating "opt-in" - left comments below along those lines though just in case it was not intended
bbrks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good 👍
- One nit about
DefaultDbConfigto ensure the default (logical) value of 1 is shown - Missing API docs for db config endpoints
Redocly previews |
bbrks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good, with one suggestion for documentation which I'll leave for @adamcfraser to review the wording of as well.
Co-authored-by: Ben Brooks <[email protected]>
adamcfraser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks great, a few minor comments/questions.
| func (i *SGIndex) createIfNeeded(ctx context.Context, bucket base.N1QLStore, options InitializeIndexOptions) error { | ||
|
|
||
| indexName := i.fullIndexName(options.UseXattrs) | ||
| if options.NumPartitions < 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere if options.NumPartitions<=1, it's treated as "do not use partitions" (i.e. the check seems to consistently be if options.NumPartitions > 1). This might be the right approach, just want to make sure we're not triggering a fatal error when options.NumPartitions isn't initialized (is zero), if it's not actually a fatal condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split the validation for this, once you get to this part of the code we do expect >= 1 to make it simpler to not have to convert everytime here and do a conversion from uninitialized to set to one. I thought this was conceptually easier because it forces the caller to set this value, so the caller decides when to actually make the indexes partitionable.
This was easier for me in testing to make sure I was passing this value through the code but I'm not committed to this approach.
integration-test/start_server.sh
Outdated
| else | ||
| # single node | ||
| docker run --rm -d --name ${SG_TEST_COUCHBASE_SERVER_DOCKER_NAME} --volume "${WORKSPACE_ROOT}:/workspace" -p 8091-8096:8091-8096 -p 11207:11207 -p 11210:11210 -p 11211:11211 -p 18091-18094:18091-18094 "${COUCHBASE_DOCKER_IMAGE_NAME}" | ||
| docker run --rm -d --name ${SG_TEST_COUCHBASE_SERVER_DOCKER_NAME} --volume "${WORKSPACE_ROOT}:/workspace" -p 8091-8096:8091-8096 -p 9102:9102 -p 11207:11207 -p 11210:11210 -p 11211:11211 -p 18091-18094:18091-18094 -p 19102 "${COUCHBASE_DOCKER_IMAGE_NAME}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| docker run --rm -d --name ${SG_TEST_COUCHBASE_SERVER_DOCKER_NAME} --volume "${WORKSPACE_ROOT}:/workspace" -p 8091-8096:8091-8096 -p 9102:9102 -p 11207:11207 -p 11210:11210 -p 11211:11211 -p 18091-18094:18091-18094 -p 19102 "${COUCHBASE_DOCKER_IMAGE_NAME}" | |
| docker run --rm -d --name ${SG_TEST_COUCHBASE_SERVER_DOCKER_NAME} --volume "${WORKSPACE_ROOT}:/workspace" -p 8091-8096:8091-8096 -p 9102:9102 -p 11207:11207 -p 11210:11210 -p 11211:11211 -p 18091-18094:18091-18094 -p 19102:19102 "${COUCHBASE_DOCKER_IMAGE_NAME}" |
I'm guessing this is needed based on other ports, but I'm not a docker expert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some extra ports based on public documentation and fixed this.
CBG-4548 allow creation of partitioned indexes
index.num_partitionsindicating how many partitions to create. By default, indexes will not be partitioned.channelsandallDocswhich scale for the number of docs and the number of channels in each document.num_index_replicastoindex.num_replicasin configuration to allow more index options to be created in the future.num_index_replicasis backward compatibleindex.num_replicasor remove the definition when unnecessary (it is set byRestTester)I think there's test coverage for everything here, but the things I am most worried about is roundtripping
/db/_configand making sure thatDbConfig.Indexdoesn't get set automatically so aGETfollowed by aPUTwould result in a mismatched configuration. Additionally, there is concern/requirement to make this work for legacy configuration.Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2998/