Move LeakTracker to production code and use it in assertions around QuerySearchContext #102179
Conversation
…uerySearchContext Part of the ref counted search hits effort requires us to correctly ref count `FetchSearchPhase`. This doesn't commit moves us one step in the direction of doing so by adding testing that ensure that `QuerySearchContext` is ref counted correctly and fixes one production code spot where it wasn't (albeit that spot worked out for other reasons anyways). This is done by moving the leak tracker to production code and making use of it selectively in case assertions are enabled.
original-brownbear
added
>test
|
Pinging @elastic/es-search (Team:Search) |
| @@ -26,14 +27,8 @@ public final class QueryFetchSearchResult extends SearchPhaseResult { | |||
| private final RefCounted refCounted; | |||
|
|
|||
| public QueryFetchSearchResult(StreamInput in) throws IOException { | |||
| super(in); | |||
There was a problem hiding this comment.
This is a super call is a noop, just fixing the duplication below.
| serviceTimeEWMA += rrfQuerySearchResult.serviceTimeEWMA(); | ||
| nodeQueueSize = Math.max(nodeQueueSize, rrfQuerySearchResult.nodeQueueSize()); | ||
| searchTimedOut = rrfQuerySearchResult.searchTimedOut(); | ||
| try (RankSearchContext rankSearchContext = new RankSearchContext(searchContext, rankQuery, rankShardContext.windowSize())) { |
There was a problem hiding this comment.
This was broken because the QuerySearchResult in RankSearchContext was leaked. Not really a bug in practice it seems but it makes sense to clean this up I think, if only to cleanly enable this kind of test.
There was a problem hiding this comment.
My understanding is the only releasable is for aggs, and these results should in theory never have aggs in them. However, I do think this is probably a bug even if we would never hit it, so thanks for fixing it :).
| @@ -245,10 +246,6 @@ public void releaseAggs() { | |||
| } | |||
| } | |||
|
|
|||
| private void close() { | |||
There was a problem hiding this comment.
This was a weird redundant method, I mainly figured this was easier to reason about GC wise when doing it inline in a lambda, but also no point in having this method when javac generates it for us :)
|
Thanks Jack! |
Part of the ref counted search hits effort requires us to correctly ref count
FetchSearchPhase. This doesn't commit moves us one step in the direction of doing so by adding testing that ensure thatQuerySearchContextis ref counted correctly and fixes one production code spot where it wasn't (albeit that spot worked out for other reasons anyways). This is done by moving the leak tracker to production code and making use of it selectively in case assertions are enabled.Mostly test changes here, review with
?w=1for easier readability :)