Repository navigation
Allow GC of closed search contexts in ES|QL - #155418
Conversation
|
Hi @dnhatn, I've created a changelog YAML for you. |
🔍 Preview links for changed docs⏳ Building and deploying preview... View progress This comment will be updated with preview links when the build is complete. |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
GalLalouche
left a comment
There was a problem hiding this comment.
Thanks @dnhatn! I have a (few) design suggestion(s) that would make this a bit cleaner. WDYT?
|
|
||
| @Override | ||
| public SearchContext searchContext() { | ||
| throw new AlreadyClosedException("ComputeSearchContext at index [" + index() + "] was already released"); |
There was a problem hiding this comment.
Nit: Extract the error message to a common helper
|
|
||
| private static class AlreadyReleasedComputeSearchContext extends ComputeSearchContext { | ||
| AlreadyReleasedComputeSearchContext(int index) { | ||
| super(index, null); |
There was a problem hiding this comment.
This feels wrong, design-wise, and an abuse of ComputeSearchContext/violating LSP (having to pass the null to a non-nullable field is the especially smelly part!). I see a few of ways around it:
- Define an
interfacebothComputeSearchContextandAlreadyReleasedComputeSearchContextimplement. That's a large blast radius though (if you want to keep it to a minimum, you can name the new interfaceComputeSearchContextand rename the class to something else). - Slightly easier, change
ComputeSearchContext'sclosemethod to also nullify theSearchContext. - Easiest, but least clean IMO: add a
protectedconstructor toComputeSearchContextthat only takes the index, and handles set thenullon its own. At least that way you avoid the smelliness mentioned above.
| final int idx = nextAddIndex++; | ||
| cse.addReleasable(() -> { | ||
| synchronized (AcquiredSearchContexts.this) { | ||
| // Allow GC of closed search contexts as soon as they are released. |
There was a problem hiding this comment.
I definitely missed this, since I assumed the overhead of maintain the SearchContext is negligible. For my own education, how heavy is this?
There was a problem hiding this comment.
It can be expensive if a shard has many segments and fields. In the customer's heap dump, many search contexts use more than 20 MB.
GalLalouche
left a comment
There was a problem hiding this comment.
LGTM, with a small suggestion.
| */ | ||
| class ComputeSearchContext implements Releasable { | ||
| private final int index; | ||
| private final SearchContext searchContext; |
There was a problem hiding this comment.
Suggestion: Mark this as @Nullable, perhaps with a comment explaining when it is nullable.
GalLalouche
left a comment
There was a problem hiding this comment.
LGTM, with a small suggestion.
|
Thanks Gal! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
ES|QL already closes search contexts as soon as compute no longer needs them. However, AcquiredSearchContexts kept those closed contexts in its global array, so their in-memory Lucene/codec structures could not be GCed. Queries target many shards could therefore retain a large amount of memory and even OOM. Relates #139693 (cherry picked from commit efa7ca9)
ES|QL already closes search contexts as soon as compute no longer needs them. However, AcquiredSearchContexts kept those closed contexts in its global array, so their in-memory Lucene/codec structures could not be GCed. Queries target many shards could therefore retain a large amount of memory and even OOM. Relates elastic#139693 (cherry picked from commit efa7ca9)
ES|QL already closes search contexts as soon as compute no longer needs them. However, AcquiredSearchContexts kept those closed contexts in its global array, so their in-memory Lucene/codec structures could not be GCed. Queries target many shards could therefore retain a large amount of memory and even OOM. Relates #139693 (cherry picked from commit efa7ca9)
ES|QL already closes search contexts as soon as compute no longer needs them. However, AcquiredSearchContexts kept those closed contexts in its global array, so their in-memory Lucene/codec structures could not be GCed. Queries target many shards could therefore retain a large amount of memory and even OOM. Relates elastic#139693
ES|QL already closes search contexts as soon as compute no longer needs them. However, AcquiredSearchContexts kept those closed contexts in its global array, so their in-memory Lucene/codec structures could not be GCed. Queries target many shards could therefore retain a large amount of memory and even OOM. Relates elastic#139693
ES|QL already closes search contexts as soon as compute no longer needs them. However, AcquiredSearchContexts kept those closed contexts in its global array, so their in-memory Lucene/codec structures could not be GCed. Queries target many shards could therefore retain a large amount of memory and even OOM.
Relates #139693