[dbnode] Register index.Query/Aggregate finalizers #1567
Conversation
| @@ -868,6 +868,7 @@ func (i *nsIndex) Query( | |||
| results.Reset(i.nsMetadata.ID(), index.QueryResultsOptions{ | |||
| SizeLimit: opts.Limit, | |||
| }) | |||
| ctx.RegisterFinalizer(results) | |||
There was a problem hiding this comment.
were we really not finalizing these? not even at the RPC layer or something weird?
There was a problem hiding this comment.
Also it looks like the finalize method will Finalize the underlying namespace ID and tags, but not the series ID. Are you sure thats safe in the context of this lifecycle? Where do the namespace IDs, tags, and series ID that end up in this thing come from?
There was a problem hiding this comment.
We were finalising these (the same way as this PR) until about a month ago, we accidentally dropped it in one of the concurrency PRs.
There was a problem hiding this comment.
Re: why is it safe?
We ensure the idents and backing bytes are independent copies of the data in the segments. i.e. look at dbnode/storage/index/results.go and dbnode/storage/index/aggregate_results.go. Here's a quick summary:
(1) the Reset() method in both files clones the nsID to start.
(2) AddDocuments() is the only way to add data to results.go and one of two ways to add data to aggregate_results.go. It also clones any data before adding to the map (or relies on the genny generated map's Set() method to do so).
(3) The only other way to add data to aggregate_results.go is the one I'm adding in #1545. In that case, we make an independent copy of the field/terms during iteration, and transfer ownership of those ident/bytes to aggregate_results.go via AddFields().
(4) Re: cleanup - both results.go and aggregate_results.go rely on Finalize() to be called (which is what this PR adds), and that internally calls Reset which in turn releases any idents/bytes/maps to the appropriate pools.
No description provided.