-
Notifications
You must be signed in to change notification settings - Fork 817
add support for bigtable as index store for integration tests #2249
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
add support for bigtable as index store for integration tests #2249
Conversation
16b5444
to
c9b35bb
Compare
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.
LGTM. I think number of parameters to various New<Service>
functions is so high now that we should refactor it to use functional options pattern (in new PR)
Yeah, right. I will add it to my todo list. |
integration/configs.go
Outdated
@@ -99,6 +105,9 @@ storage: | |||
aws: | |||
dynamodbconfig: | |||
dynamodb: {{.DynamoDBURL}} | |||
bigtable: |
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.
By contract, ChunksStorageConfig
should be specular to ChunksStorageFlags
, so if you do a change to ChunksStorageConfig
you should do the same change to ChunksStorageFlags
and viceversa.
However, these configs where designed to be ready to use and should not have mixed config (either the storage is AWS or BigTable). I would suggest to:
- Rename
ChunksStorageConfig
toChunksStorageDynamoDBConfig
- Rename
ChunksStorageFlags
toChunksStorageDynamoDBFlags
- Create
ChunksStorageBigtableConfig
andChunksStorageBigtableFlags
with the config you want
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.
It seems I can get rid of this change completely because it is not used in the new test that I added.
integration/e2ecortex/services.go
Outdated
) | ||
} | ||
|
||
func NewQuerier(name string, consulAddress string, flags map[string]string, image string) *CortexService { | ||
return NewQuerierWithConfigFile(name, consulAddress, "", flags, image) | ||
func NewQuerier(name string, consulAddress string, flags map[string]string, image string, envVars map[string]string) *CortexService { |
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'm not sure it's worth adding envVars
to all such functions, considering it's not a commong use case and you could just call service.SetEnvVars()
in the single integration test where you need it. What's your take?
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 had started with that approach first but then it made me feel someone could miss setting that env var, while setting all the params in the function is explicitly done so less of a chance to miss it. But I think we would mostly continue using dynamo for all the other test so I can use that function for now. We can refactor it again later if we see more needs for setting environment variables.
Thanks!
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.
then it made me feel someone could miss setting that env var
It would miss anyway if that person is not aware of the required env var. If you want to guaraantee it you should take bigtableAddress
in input, like we do for consulAddress
, but to me looks a bit over-engineered adding bigtableAddress
to each of such functions given it's only required by 1 single scenario (contrary to consul).
integration/all_stores_test.go
Outdated
|
||
// here we are starting and stopping table manager for each index store | ||
// this is a workaround to make table manager create tables for each config since it considers only latest schema config while creating tables | ||
for i := range storeConfigs { |
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 a bit hacky. Why don't we run the table manager only once, but with a schema config containing both storages (two entries in the same schema config)? Doing it with buildSchemaConfigWith()
may be engineered (would require extra refactoring I'm not sure it's worth). You could just rollback buildSchemaConfigWith()
and have a static schema config defined in this file.
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 is done because TableManager
only uses latest config to create tables i.e it does not consider older configs while creating tables. There is a PR open to fix this behaviour in Cortex. See #1446
We have not seen someone complain about it because we add only one schema at a time to cortex as and when needed but here I am setting multiple schema configs at a time.
Does it make sense?
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.
Yes. I wasn't aware of the issue. Could you add a // TODO
comment referencing the PR and saying that we can merge two two schema configs into 1 and just run the table-manager once once that PR is merged, please?
integration/all_stores_test.go
Outdated
require.NoError(t, err) | ||
require.Equal(t, 200, res.StatusCode) | ||
|
||
// Query the series both from the querier and query-frontend (to hit the read path). |
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.
// Query the series both from the querier and query-frontend (to hit the read path). | |
// Query back the series. |
integration/all_stores_test.go
Outdated
require.Equal(t, 200, res.StatusCode) | ||
|
||
// Query the series both from the querier and query-frontend (to hit the read path). | ||
c, err = e2ecortex.NewClient("", querier.HTTPEndpoint(), "", "user-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.
This test should hit the storage, but how is it guaranteed that the series have been flushed to the storage and offloaded from the ingesters memory at this point?
I've the feeling we're not hitting the backend store neither on the write and read path, because the chunks are just kept in the ingesters memory.
it also adds a simple test for all the supported index stores in integration tests Signed-off-by: Sandeep Sukhani <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Sandeep Sukhani <[email protected]>
Signed-off-by: Sandeep Sukhani <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
9288ae3
to
b2ad1d9
Compare
Signed-off-by: Marco Pracucci <[email protected]>
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.
Thanks @sandeepsukhani! LGTM. As agreed offline, I pushed a couple of small changes.
…project/cortex#2249) * add support for bigtable as index store for integration tests it also adds a simple test for all the supported index stores in integration tests Signed-off-by: Sandeep Sukhani <[email protected]> * Added comment to Bigtable image Signed-off-by: Marco Pracucci <[email protected]> * changes suggested from PR review Signed-off-by: Sandeep Sukhani <[email protected]> * changes suggested from PR review Signed-off-by: Sandeep Sukhani <[email protected]> * Simplified client Signed-off-by: Marco Pracucci <[email protected]> * Renamed test file Signed-off-by: Marco Pracucci <[email protected]> Co-authored-by: Marco Pracucci <[email protected]>
What this PR does:
Add support for Bigtable in integration tests using https://github.com/Shopify/bigtable-emulator
It also adds a simple test for all the supported index stores in integration tests while keeping
dynamo-db
as default for rest of the tests.