Store-gateway: introduce chunkRangeReader #4209
Conversation
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>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
partitioner Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
| rLen = int(diff) | ||
| } | ||
| } | ||
| rangeBuf := make([]byte, rLen) |
There was a problem hiding this comment.
[for a follow up discussion] Have you experienced with a memory pool here?
There was a problem hiding this comment.
i ran some tests with a SlabPool, but i don't think the results were very reliable. The overall theme was that running with 16K slab size was too small and there were about 1/4 of allocations happening because the slab was too small for the range bytes. I also tried with 1.6M and that was too large, i suspect because of too few selected series.
I haven't tried with any values between 1.6M and 16K, maybe the sweet spot is somewhere there.
There was a problem hiding this comment.
We can reconsider a pool here later, if we see this popping up in profiles.
| rangeEntry int | ||
| } | ||
|
|
||
| type bucketChunksRangesReader struct { |
There was a problem hiding this comment.
A suggestion to unit test this. We don't have to get mad, but a unit test for the basic case may be nice.
Instead of storing *bucketBlock, we keep (a) the partitioner (b) a reference to the function chunkRangeReader() (c) the logger (d) the block ID. In the test we override the partitioner and chunkRangeRange(). The chunkRangeRange() returns a simple reader that returns the sequence of bytes in the range 0-254 each time you read bytes from it. When asserting the actual chunks data read we assert that the bytes range fetched match the expected one (you can test it with very small chunk ranges, so that the total size of data read is <= 254 bytes).
| // parse tries to parse the ranges in the partial set with the bytes it has in rawRanges. | ||
| // If the bytes for any of the ranges aren't enough, then parse will return an underfetchedChunksRangeIdx | ||
| // and will correct the length of all ranges which had a understimated length. | ||
| // Currently, parse will only correct the length of the last chunk, since this is the only chunk |
There was a problem hiding this comment.
I don't think this is entirely true. We start from the assumption that chunks are sorted in the segment file, but in the logic filling the length we clamp to 16000 bytes if the length we compute is longer than that. In the extreme case the real chunk is even longer, we may have underfetched a chunk which is not necessarily the last one.
| // An error is returned when gBytes are malformed or when more than the last chunk is incomplete. | ||
| // | ||
| //nolint:unused // dead code while we are working on PR 3968 | ||
| func parseRange(rBytes []byte, chunks []storepb.AggrChunk) (allChunksComplete bool, lastChunkLen uint32, totalRead int, _ error) { |
There was a problem hiding this comment.
This function is self contained and should be easy to unit test.
| // if the data in rawRanges is invalid. | ||
| // | ||
| //nolint:unused // dead code while we are working on PR 3968 | ||
| func (s partialSeriesChunksSet) parse() ([]underfetchedChunksRangeIdx, error) { |
There was a problem hiding this comment.
This looks testable, given partialSeriesChunksSet is self contained and doesn't rely on any external dependancy.
Signed-off-by: Dimitar Dimitrov dimitar.dimitrov@grafana.com
What this PR does
This is a followup of PR #4174 and upstreams another part of #3968
This PR introduces means to parse the raw byte slices of chunk ranges into
storepb.AggrChunk. Currently these are not used, but will be in a subsequent PR.The load of ranges happens via an alternative implementation of the existing
bucketChunkReaderandbucketChunkReaders. The new implementation is calledbucketChunkRangesReaderandbucketChunkRangesReaders.The overall flow of loading chunk ranges is the following:
seriesChunkRefsSetsame as nowpartialSeriesChunksfor each series, which holds both the raw byte ranges for each series' groups and the parsed chunkspartialSeriesChunksis introduced in this PRpartialSeriesChunks.rawRangespartialSeriesChunks.rawRangespartialSeriesChunks.parse()which reads all collected raw byte slices and tries to parse all the chunks from them intopartialSeriesChunks.parsedChunks.parse()may not be able to parse a whole range because it was underfetched. In that caseparse()will return aunderfetchedChunksRangeIdxfor each underfetched rangeunderfetchedChunksRangeIdxfor each series and refetch the ranges from the bucket using thebucketChunkRangesReadersunderfetchedChunksRangeIdxback topartialSeriesChunks.reparse()which attempts to parse only that individual range of chunksWhich issue(s) this PR fixes or relates to
Related to #3939
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]