Move DynamoDB index tests to cover other storage backends, fix bunch of Cassandra issues.#783
Merged
bboreham merged 6 commits intocortexproject:masterfrom Apr 4, 2018
Merged
Conversation
tomwilkie
changed the title
Contributor
|
I find it very hard to review a PR like this.
|
- Factor out `forAllFixtures` function, to run a test over all the storage types. - Move some utils functions to pkg/chunk/storage/utils_test.go.
…results; server doesn't offer this facility in a consumable fashion.
… causes no ends of issues. Instead, just have the DynamoDB implementation work it out for itself.
Doesn't run by default, as it depends on an external Cassandra cluster. See pkg/chunk/cassandra/fixtures.go for details on how to run.
- Add errors.WithStack on all error coming back from Cassandra. - Implement server-side value filtering. - Correctly implement prefix queries. - Don't call the test table "table", as this is a reserved word in CQL. - Don't use Cassandra batches for writes, they don't do what you think they do. - Correctly honor QueryPage callback value <--- this was the cause of the bug I was chasing...
Contributor
Author
|
@bboreham PTAL, I've tried to make this easier to follow for you. |
bboreham
approved these changes
Apr 4, 2018
|
|
||
| /* BigTable only seems to support regex match on cell values, so doing it | ||
| client side for now | ||
| readOpts := []bigtable.ReadOption{ |
Contributor
There was a problem hiding this comment.
I'm unclear why this code is commented out. Is it an example of what you'd like to do but doesn't work?
Contributor
Author
There was a problem hiding this comment.
Yeah, pretty much - I'm hoping to get some input from the Bigtable team on this. Seems weird we can do regex matches on values, but not equality matches.
Normally don't like to leave this kind of mess in there, and can remove it if you like. I'll move it to another PR that will sit open otherwise though - I need somewhere to point them at.
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.
Firstly, move some tests around so we get some coverage on the Cassandra backend (and BigTable).
Next, refactor out QueryPage lastPage param, its a dumb interface. The various backends can do a better job here; it only affects the AWS backend anyway.
Finally, fix a bunch of issue with the QueryPage implementation for Cassandra. Details in commits.