Repository navigation
SOLR-17539: Add query.enableDocValuesIteratorCache to control the DocValues iterator reuse cache - #4998
Open
nick-boss-tech wants to merge 6 commits into
Open
SOLR-17539: Add query.enableDocValuesIteratorCache to control the DocValues iterator reuse cache#4998nick-boss-tech wants to merge 6 commits into
nick-boss-tech wants to merge 6 commits into
Conversation
…luesIteratorCache Use org.apache.solr.client.solrj.request.SolrQuery (the class moved packages) and apply spotless formatting to the realtime-get block.
…Cache takes effect on core reload
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖 (posted on behalf of Nick Shanin)
https://issues.apache.org/jira/browse/SOLR-17539
What happens today
After the upgrade to Solr 9.6, users reported increased memory usage traced to the request-scoped DocValues iterator reuse cache. A wide field list (for example
fl=*) pins iterators in the cache for fields the query never benefits from, and there is no way to turn the reuse off.What this change does
It adds a
query.enableDocValuesIteratorCacheoption in solrconfig.xml, defaulting totrue, so current behavior is unchanged unless an operator opts out. When it is set tofalse, the DocValuesIteratorCache for the request retains no per-field suppliers, so iterators are not held across field loads. The two request paths that build the cache (SolrDocumentFetcher and RealTimeGetComponent) now go through SolrDocumentFetcher.createDocValuesIteratorCache(), which honors the setting. The option can also be changed through the Config API, taking effect on the core reload the API triggers, and it is documented in the ref guide; both bundled configsets set it explicitly. This also answers the getDocValues() concern raised on the ticket: with the switch off, the supplier is rebuilt per call, so the getDocValues() OOM exposure goes away on the two changed paths; with the switch on (the default), nothing changes.Proof
On this branch: TestDocValuesIteratorCache passes 2 of 2 (one test on the cache's retention behavior itself, one on the option taking effect through solrconfig), SolrCoreTest passes 9 of 9, and TestConfigOverlay passes 2 of 2. The pre-fix proof is inconclusive by construction: the cache tests call isCaching(), isDocValuesIteratorCacheEnabled(), and createDocValuesIteratorCache(), which this PR adds, so they do not compile against the base code. The evidence for the change rests on those head tests passing and on the default preserving the old behavior.
A choice to check
The alternative to an on/off switch would be to keep the cache always on and instead bound it, or to skip caching for fields pulled in by glob expansion. I chose the switch: it gives the operator reporting the memory problem a predictable remedy, and the default leaves everyone else untouched. A bounded or selective cache is more machinery, and it still keeps bookkeeping for fields a wide query never reuses. If you would rather fix the cache itself, I can take that direction instead.
Limits
Disabling the cache trades memory for speed: with caching off, every getSupplier call builds a new FieldDocValuesSupplier, which allocates four arrays sized to the number of segments, so the cost lands per field per document instead of iterators being reused. The heap effect of the switch is inferred from the code, not measured: the ticket reports heap growth from 4 GB to 6 GB, and no before/after measurement is part of this proof. UpgradeCoreIndex still builds a caching instance unconditionally; that one-off upgrade path is out of scope here. The setting is per-core configuration, not per-request, so it cannot be toggled for a single query.
Changelog: changelog entry under changelog/unreleased (added)
AI assistance
AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.