storage,logicalplan: lazy iterator allocation with batching for range… - #721
Conversation
|
|
yeya24
left a comment
There was a problem hiding this comment.
Overall I think we might miss some fuzz tests. Can we revisit the existing fuzzy tests in this repo and make sure it has set batch size enabled so this code change is exercised? I want to understand if there is any edge case we might hit that breaks the series batch optimizer
|
|
||
| if o.opts.IsInstantQuery() { | ||
| scanner.buffer.Reset(math.MaxInt64, 0) | ||
| scanner.iterator = nil |
There was a problem hiding this comment.
One further improvement. If we want to reduce allocation further, we don't have to eagerly allocate the iterator upfront. Can reuse the iterator created for the previous scanner. Need to be careful if it works for us.
for i := firstSeries; i < lastInBatch; i++ {
if o.scanners[i].iterator == nil {
o.scanners[i].iterator = o.scanners[i].rawSeries.Iterator(nil)
o.scanners[i].buffer = o.newBuffer(ctx)
}
}
There was a problem hiding this comment.
Thanks. Added the TODO.
|
I merged #718 which seems to have created a conflict but it should be easy to resolve. That PR removes a few fields from the matrix selector. |
… functions Two changes that together fix ruler OOM from unbounded iterator allocation: 1. logicalplan: enable series batching for range vector functions The SelectorBatchSize optimizer previously disabled batching for ALL FunctionCall nodes. Range vector functions (rate, increase, present_over_time, etc.) are per-series independent — each series' result depends only on that series' own range data. This change only disables batching for instant vector functions (no matrix arg) while allowing it for range vector functions. 2. storage: defer iterator and buffer allocation to batch time Previously, loadSeries() called s.Iterator(nil) and newBuffer() for ALL series upfront. With high-cardinality tenants (4.5M series), this allocates 4.5M merge iterators simultaneously, causing 22GB+ heap and OOM kills on ruler pods. Move iterator and buffer allocation from loadSeries() to Next(), where they are initialized per-batch. With seriesBatchSize=256, only 256 merge iterators exist at any time instead of the full series count. Together: the optimizer enables batching for the OOM-causing query patterns, and the lazy allocation makes batching actually reduce peak memory from O(N) to O(batchSize). Signed-off-by: Paurush Garg <paurushg@amazon.com>
44818b6 to
84c1a8a
Compare
@fpetkovski Thanks. Updated. |
@yeya24 Thanks. I added SelectorBatchSize: 256 to all 4 fuzz tests. |
Problem
Ruler instant queries with high cardinality hit OOM because matrixSelector.loadSeries() allocates iterators and buffers for ALL series upfront. With N series, this creates N merge iterators simultaneously. In Prometheus, this doesn't happen because its engine processes one series at a time and releases between series. The Thanos engine holds all iterators simultaneously.
Fix
Move s.Iterator(nil) and newBuffer() from loadSeries() to Next(). Iterators are now created per-batch (controlled by seriesBatchSize). For instant queries, iterators are released after each batch (scanner.iterator = nil).
Rewrote the SelectorBatchSize optimizer - to enable batching for:
Benchmark
Instant queries (3K series, SelectorBatchSize=256): #721 (comment)
Range queries (3K series, SelectorBatchSize=256): #721 (comment)