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.

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

Fetching large chunks #2956

Merged
merged 8 commits into from
Jul 30, 2020
Merged

Fetching large chunks #2956

merged 8 commits into from
Jul 30, 2020

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Jul 29, 2020

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This PR fixes issue with fetching chunks that are bigger than 16000 bytes. Under some circumstances it is possible to come across chunks like that, and under some conditions Thanos may currently fail to fetch such chunks. This PR adds fetching of remaining chunk data from the bucket.

Verification

Added unit test. Also tested manually against real block found in our test cluster.

Fixes issue #2945.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, amazing. I have only one non blocking comment. Otherwise LGTM!


// If we didn't fetch enough data for the chunk, fetch more. This can only really happen for last
// chunk in the list of fetched chunks, otherwise partitioner would merge fetch ranges together.
r.mtx.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

Worth to mention what this mutex is saving from. Probably changing name would be nice. I know it was like this before, but maybe we can improve that part. I assume it's for stats, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's both for stats and storing chunks. I will document and rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also chunkBytes.

Also document mutex usage.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@bwplotka bwplotka merged commit ee52915 into thanos-io:master Jul 30, 2020
@bwplotka
Copy link
Member

Thanks!

@pracucci
Copy link
Contributor

pracucci commented Aug 3, 2020

I'm joining the party very late, but good job @pstibrany ! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants