perf(blooms): always return bloom pages to allocator #13288
Conversation
Signed-off-by: Owen Diehl <[email protected]>
pkg/bloomgateway/processor.go
Outdated
|
|
||
| defer func() { | ||
| for i := range bqs { | ||
| if bqs[i] == nil { | ||
| continue | ||
| } | ||
| bqs[i].Close() | ||
| } | ||
| }() |
There was a problem hiding this comment.
Maybe I miss something, but we don't seem to close the block remaining block queriers any more in case one p.processBlock() fails, since concurrency.ForEachJob() returns on first error.
There was a problem hiding this comment.
The closing has moved into processBlock itself so we can cleanup & return to the buffer pool as blocks individually finish, not after they all do. This also helps prevent issues if/when we start blocking on pool accesses where further blocks could hang waiting for Gets from the buffer pool that haven't been returned from previous blocks.
pkg/bloomgateway/processor.go
Outdated
| var errs multierror.MultiError | ||
| errs.Add(err) | ||
| errs.Add(bq.Close()) | ||
| err = errs.Err() |
There was a problem hiding this comment.
That looks wrong, since on line 141 you add err to the multi-error.
There was a problem hiding this comment.
That's capturing the err from the function signature and adding it to the multi-error (which can now have a main error from the body of the function call and/or an error from cleanup.
Signed-off-by: Owen Diehl <[email protected]>
The slice of block queriers can contain `nil` values, which causes nip pointer dereference when calling `Close()` on them. The regression was introduced with PR #13288 Signed-off-by: Christian Haudum <[email protected]>
No description provided.