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.

Sign up for GitHub

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

Jump to bottom

Blocks querier: don't reencode TSDB chunks to Cortex chunks during querying #2061

Merged
merged 13 commits into from
Feb 18, 2020
Merged

Blocks querier: don't reencode TSDB chunks to Cortex chunks during querying #2061

merged 13 commits into from
Feb 18, 2020

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Jan 31, 2020 •
edited
Loading

This rewrites BlocksQuerier to implement Queryable interface. It does that by implementing storage.SeriesSet on top of returned streamed response from StoreClient.Series call.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pstibrany pstibrany mentioned this pull request Feb 3, 2020
Merged
3 tasks
@pstibrany
Avoid reencoding TSDB chunks to Cortex chunks.
619e1c9 
Blocks querier now report samples from TSDB chunks directly,
without reencoding them to Cortex chunks.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Removed unused method.
b881e6a 
It was used to generate invalid data, but we no longer do the conversion
where this could be detected.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Removed duplicate error iterator.
777d48d 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany pstibrany marked this pull request as ready for review February 17, 2020 10:35
@pstibrany
Copy link
Contributor Author

As #2060 has been merged, this PR is now rebased and ready for review.

/cc @pracucci @thorfour

pracucci
pracucci reviewed Feb 17, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job @pstibrany! Overall, looks good to me. I left few comments to see if we can simplify a bit the implementation.

pkg/querier/block.go Show resolved Hide resolved
pkg/querier/block.go Outdated Show resolved Hide resolved
pkg/querier/block.go Show resolved Hide resolved
pkg/querier/block.go
chunks = append(chunks, chunk.NewChunk(userID, client.Fingerprint(lbls), lbls, ch, model.Time(c.MinTime), model.Time(c.MaxTime)))
// blockQuerierSeriesIterator implements a series iterator on top
// of a list of time-sorted, non-overlapping chunks.
type blockQuerierSeriesIterator struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we're copy-pasting Thanos code here, I would suggest to ask @bwplotka if they're willing to accept a PR to expose newChunkSeriesIterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, it doesn't seem worth the troubles for 50 lines of code to me.

But since I just found a bug in that iterator code (it calls At() before calling Next(), which it shouldn't), I may try to send patch with the bugfix and making it public. (But it's still low prio)

pkg/querier/block.go Show resolved Hide resolved
@pstibrany
Added unit test for blockQuerierSeriesSet.
74341a8 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Precompute labels to avoid repeated allocations if Labels is
882bb77 
called multiple times.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Make sure blockQuerierSeriesIterator calls Next before any At operati…
c9d33ce 
…on on iterators it owns.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Unimplemented method that Cortex doesn't use.
529fda1 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Read entire response before returning series set. Don't use streaming.
5a55e62 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Copy link
Contributor Author

Thank you for your review Marco. I've addressed all feedback, except sending PR to Thanos (but I believe I've fixed the implementation of iterator).

pracucci
pracucci reviewed Feb 18, 2020
View reviewed changes
pkg/querier/block.go Outdated Show resolved Hide resolved
pkg/querier/block_test.go Outdated Show resolved Hide resolved
pkg/querier/block_test.go Outdated Show resolved Hide resolved
pkg/querier/block_test.go Show resolved Hide resolved
pkg/querier/block.go Outdated Show resolved Hide resolved
pkg/querier/block.go Show resolved Hide resolved
@pstibrany
Use labels.Builder to build labels.
ff8895c 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Test that chunks get sorted, verify increasing timestamp of samples.
821687e 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Renamed test.
382dac6 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Use single Recv() call.
f1a98fd 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Make sure we've got all expected samples
191d611 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pracucci
pracucci approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @pstibrany for addressing my feedback! Changes LGTM and affect only the blocks storage, so should be safe to merge.

pkg/querier/block.go Show resolved Hide resolved
jtlisi
jtlisi approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

thorfour
thorfour approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@thorfour thorfour left a comment

Choose a reason for hiding this comment

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

shipit!

@pracucci pracucci merged commit d6e9f71 into cortexproject:master Feb 18, 2020
@pstibrany pstibrany deleted the tsdb-dont-reencode-chunks-to-cortex-chunks branch February 18, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thorfour thorfour thorfour approved these changes

@jtlisi jtlisi jtlisi approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pstibrany @pracucci @jtlisi @thorfour