MQE: Fix panic in loading too many samples #10261
Merged
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
BucketedPool would create pools in powers of two up to a set `maxSize`. That is, if `maxSize` isn't a power of two itself, then the maximum bucket would be less than the maxSize. However, when requesting a slice from the pool we were only checking against the maxSize, and not whether a bucket existed for that size. Instead calculate the bucketIndex and check if that exists before using it.
This is to guarantee they work with the ring buffers which expect slices to always be power of two. The limiting pool still protects us from requesting an unreasonable amount of points with the MemoryConsumptionTracker.
1 task
pracucci
reviewed
Dec 17, 2024
|
(this will need backporting to r321) |
pracucci
approved these changes
Dec 17, 2024
pracucci
reviewed
Dec 18, 2024
jhesketh
added a commit
that referenced
this pull request
Dec 18, 2024
* MQE: Fix panic in loading too many samples BucketedPool would create pools in powers of two up to a set `maxSize`. That is, if `maxSize` isn't a power of two itself, then the maximum bucket would be less than the maxSize. However, when requesting a slice from the pool we were only checking against the maxSize, and not whether a bucket existed for that size. Instead calculate the bucketIndex and check if that exists before using it. * Use a power of two size max bucket * MQE: Ensure BucketedPool always returns slices of power two This is to guarantee they work with the ring buffers which expect slices to always be power of two. The limiting pool still protects us from requesting an unreasonable amount of points with the MemoryConsumptionTracker. * Fix tests * Correctly return slices to their respective buckets or discard them * Extra tests * Address review feedback * Remove unncessary slice length check (cherry picked from commit 34a24b1) Co-authored-by: Joshua Hesketh <joshua.hesketh@grafana.com>
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.
What this PR does
This PR solves two bugs:
Bug 1:
BucketedPool would create pools in powers of two up to a set
maxSize. That is, ifmaxSizeisn't a power of two itself,then the maximum bucket would be less than the maxSize.
However, when requesting a slice from the pool we were only checking against the maxSize, and not whether a bucket existed for that size. Now, instead, calculate the bucketIndex and check if that exists before using it.
This would cause panic's in range vectors of more than 65,536 samples.
This problem was introduced in #10199
Bug 2:
If a requested slice size exceeded the size of the pools it would just be returned as the requested size. This caused a bug in the buffers which require the slices to be powers of two.
This PR ensures that the returned slice is always of power two. This means we will return a slice greater than the
MaxExpectedPointsPerSeries, but it won't be from a pool (and similarly won't be returned to a pool). We are protected by theMemoryConsumptionTrackerfor any unreasonable requests.This was introduced in #9664
Originally this would cause panic's in sub-queries selecting more than 65,536 samples, but since #10199 it returns just errors.
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.