store-gateway: fix loading of series with chunks in multiple segment files #5875
Conversation
This file contains 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
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
added
bug
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
pracucci
approved these changes
Aug 31, 2023
| @@ -1012,7 +1012,7 @@ func (s *loadingSeriesChunkRefsSetIterator) symbolizedSet(ctx context.Context, p | |||
| } | |||
| case !s.strategy.isNoChunkRefs(): | |||
| clampLastChunkLength(symbolizedSet.series, metas) | |||
| series.chunksRanges = metasToRanges([][]chunks.Meta{metas}, s.blockID, s.minTime, s.maxTime) | |||
| series.chunksRanges = metasToRanges(partitionChunks(metas, 1, 1), s.blockID, s.minTime, s.maxTime) | |||
There was a problem hiding this comment.
[for another PR] We can simplify partitionChunks() to not take targetNumRanges, minChunksPerRange at all.
There was a problem hiding this comment.
I was planning to remove partitionChunks and chunks ranges altogether in a future PR. I wanted to do that this week, but this bug took too much time to find and fix, so it will probably roll into next week. Should I still simplify partitionChunks before that its removal?
There was a problem hiding this comment.
Should I still simplify partitionChunks before that its removal?
No :)
dimitarvdimitrov
deleted the
dimitar/st-gw/add-test-for-chunks-in-multiple-segemtn-files
branch
grafanabot
pushed a commit
that referenced
this pull request
Aug 31, 2023
…files (#5875) * Add test for series with chunks in multiple segment files Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Partition chunks from multiple segment files into multiple ranges Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Remove redundant upload Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Remove test tool Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> --------- Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> (cherry picked from commit 85be770)
grafanabot
pushed a commit
that referenced
this pull request
Aug 31, 2023
…files (#5875) * Add test for series with chunks in multiple segment files Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Partition chunks from multiple segment files into multiple ranges Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Remove redundant upload Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Remove test tool Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> --------- Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> (cherry picked from commit 85be770)
colega
added a commit
that referenced
this pull request
Sep 1, 2023
…ltiple segment files (#5891) * store-gateway: fix loading of series with chunks in multiple segment files (#5875) * Add test for series with chunks in multiple segment files Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Partition chunks from multiple segment files into multiple ranges Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Remove redundant upload Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Remove test tool Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> --------- Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> (cherry picked from commit 85be770) * Update changelog entry Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Update changelog entry Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> * Update CHANGELOG.md Co-authored-by: Oleg Zaytsev <mail@olegzaytsev.com> --------- Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Co-authored-by: Oleg Zaytsev <mail@olegzaytsev.com>
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The bug
Fixes a bug introduced in #5816 (part of r253 and 2.10.0-rc.0) where series with chunks in multiple segment files wouldn't be fetched properly. The cause of the bug was that after 5816 all chunks were in the same chunks range. Since the ID of the segment file is kept once per chunks range, this lead to having chunks from multiple segment files in the same chunks range. The chunks from the second segment file would b e fetched from the first segment file, leading to invalid offsets and overlapping chunks.
The fix
The fix was trivial - reintroduce the usage of
partitionChunkswhich used to partition chunks into multiple ranges based on their segment files. This was the base before 5816 as well.The side effects in tests
We didn't catch this bug because we don't test large enough blocks that have multiple segment files and series that have multiple chunks. To introduce a test that checks these I had to introduce a few changes to the test setup and the assertions.
Previously the tests in series_refs_test.go would each assert on the chunks' MinTime, MaxTime and their reference. The correct values were found when first writing the tests. This limited how many chunks we can assert against (it's infeasible to hardcode the refs of 100K chunks); see the test cases commented with "There is still no easy way to assert on the refs of 100K chunks, so we skip them."
Now we run prometheus block query code to get the chunk MinTime, MaxTime, and references instead of hardcoding them in the tests. This allows to run with many more chunks and segment files and still do exact assertions. The tradeoff is that we now don't have that strict assertions on the chunk length estimation. I think this is ok, since the estimation doesn't play a huge role in chunks fetching and bugs in it aren't critical.
The changes in the tests will also make the future removals of fine-grained chunks caching easier.
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]