Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces stubbed index creation functions for test purposes to improve integration test performance. Creating and destroying indexes on N1QL nodes is time-consuming and causing tests to timeout after 1 minute during full integration test runs.
Changes:
- Adds injectable
InitializeIndexesFunctoDatabaseInitManagerandDatabaseInitWorkerfor test stubbing - Introduces noop index creation functions (
getNoopInitializeIndexesandgetNoopBlockingInitializeIndexes) to avoid actual index operations - Fixes bug where
DatabaseContext.AsyncIndexInitManagerwas nil when database started offline, causing potential panics
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rest/indextest/index_init_api_test.go | Updates tests to use stubbed index creation functions and replaces manual polling with helper function |
| rest/database_init_manager.go | Adds InitializeIndexesFunc injection mechanism for test stubbing |
| db/database.go | Moves AsyncIndexInitManager initialization to NewDatabaseContext to prevent nil panics |
| base/util_testing.go | Improves error reporting in DropAllIndexes to show remaining indexes on failure |
| db.RequireBackgroundManagerState(t, rt.GetDatabase().AsyncIndexInitManager, db.BackgroundProcessStateCompleted) | ||
| } | ||
|
|
||
| // getNoopBlockingInitializeIndexes is a replacement initialize index function that blocks forever. It is cancelled when DatabaseInitManager.Cancel is called. Used to avoid churn on n1ql nodes. |
There was a problem hiding this comment.
Corrected spelling of 'n1ql' to 'N1QL'.
| } | ||
|
|
||
| // getNoopInitializeIndexes is a replacement initialize index function that returns immediately. Used to avoid churn on | ||
| // on n1ql nodes. |
There was a problem hiding this comment.
Corrected spelling of 'n1ql' to 'N1QL'.
Suggested change
| // on n1ql nodes. | |
| // on N1QL nodes. |
| } | ||
|
|
||
| // getNoopInitializeIndexes is a replacement initialize index function that returns immediately. Used to avoid churn on | ||
| // on n1ql nodes. |
There was a problem hiding this comment.
Duplicate word 'on' should be removed.
Suggested change
| // on n1ql nodes. | |
| // n1ql nodes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CBG-5160 stub index creation for test purposes
Creating and destroying indexes takes a lot of time for N1QL nodes. Sometimes they give up after 1 minute when running full integration tests.
I don't think to test the behavior of the
AsyncIndexInitManagerthat we need to actually create the indexes, since we aren't verifying that they are being created. There is code elsewhere that does test whether these are created.Note:
SetInitializeIndexesFuncto rest package or something that isn't dropping indexes each time.DatabaseContext.AsyncIndexInitManagerwould be nil if the database was started offline and cause a panic.indextestwhich are candidates for mocking, especiallyTestChangeIndexPartitions. The first half of this test would work for mocking, but the second part of the test what tests_post_upgraderemoval process can't be tested using this framework, so I did not modify it.I'm open to this not being the right thing to do, but I'm reluctant to increase the wait time for indexes to be created above 1 minute because I think it could take a really long time and our integration tests are now pushing two hours. I don't think the real creation of these indexes is that useful, we do test the partitioned index creation in db/indextest package. I think it would be fine to leave a small number of these tests preesent.
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/268/