Toni Cárdenas
Toni Cárdenas
Thanks @adonovan. > This proposal seems reasonable to me. A few thoughts: > > Given that absolute line numbers in expectations are inherently fragile, it might be more robust to...
Thank you. I've amended the proposal as discussed.
@aclements Sounds good to me! Is there any kind of "official" approval (or rejection) I should be waiting for before working on this?
I expected the revert [69ca33a](https://github.com/grafana/mimir/pull/13609/commits/69ca33a9dd95354e5ae696a021f1f44efbf263d5) to make the regression test I added to fail, but it doest in CI? It fails locally though: ``` go test ./pkg/ingester/ -run=TestIngesterNoRW2MetadataRefLeaks -v ===...
[There's the regression failure](https://github.com/grafana/mimir/actions/runs/19627865012/job/56200440424?pr=13609), after reverting the fix: ``` --- FAIL: TestIngesterNoRW2MetadataRefLeaks (0.41s) ingester_test.go:9169: Error Trace: /__w/mimir/mimir/pkg/ingester/ingester_test.go:9169 Error: reference leak detected Test: TestIngesterNoRW2MetadataRefLeaks Messages: addr: 0xc000246420 ``` I'll now reapply...
The initial approach of relying on GC (not) collecting supposedly unreferenced objects to detect leaks proved to be too flaky in practice. On [d9afc39](https://github.com/grafana/mimir/pull/13609/commits/d9afc3968f123a98a4f74fc70f86f595685250b2), we instead allocate the instrumented buffers...
Testing [ff97b8c](https://github.com/grafana/mimir/pull/13609/commits/ff97b8c7bf255a13c8bf240b8e3795f5dc030ebc) under some real load, it's apparent that we're not calling `WriteRequest.FreeBuffer` everywhere we should. We need to plug those leaks before merging this.
[ef0ccb6](https://github.com/grafana/mimir/pull/13609/commits/ef0ccb6abfda6bba001bbd887b169c1df06b25a4) fixes the leaks.