Invalidate exact bitset filter cache keys on reader close - #22498
Conversation
PR Reviewer Guide 🔍(Review updated until commit 413decd)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 413decd Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 2e255f5
Suggestions up to commit 9452a6c
Suggestions up to commit a2036c8
Suggestions up to commit d3c5ed6
Suggestions up to commit 511426b
|
|
❌ Gradle check result for b79ef1e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
b79ef1e to
511426b
Compare
|
Persistent review updated to latest commit 511426b |
|
❌ Gradle check result for 511426b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 511426b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
@sgup432 please review this as well. |
511426b to
d3c5ed6
Compare
|
Persistent review updated to latest commit d3c5ed6 |
|
❌ Gradle check result for d3c5ed6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
The node-level bitset filter cache deferred cleanup of entries from closed readers to a periodic sweep over Cache.keys(). That iterator walks the LRU linked list without holding the LRU lock, so a concurrent cache hit relinking an entry to the head of the LRU list can cause the sweep to skip it; because the sweep drains the stale marks after each pass and a reader closes only once, a skipped entry is retained until size-based eviction reaches it - potentially forever. Instead of sweeping, track every key cached for a reader and invalidate exactly those keys, synchronously, when the reader closes - matching the fielddata cache fix in opensearch-project#22491. This removes the periodic BitsetCacheCleaner, its indices.cache.bitset.cleanup_interval setting, and the stale-mark bookkeeping entirely. Signed-off-by: Josh Wilson <joshuaw@squareup.com>
d3c5ed6 to
a2036c8
Compare
|
Persistent review updated to latest commit a2036c8 |
|
@andrross I believe this is another concerning case that can cause permeant misses (until the cache is full) if hit. Since these also link to unaccounted for objects like Query, ValueWrapper, etc I think it may also be able to grow larger than the max cache size. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22498 +/- ##
============================================
- Coverage 71.48% 71.47% -0.01%
+ Complexity 77022 77010 -12
============================================
Files 6156 6156
Lines 358482 358454 -28
Branches 52246 52244 -2
============================================
- Hits 256255 256213 -42
- Misses 81810 81877 +67
+ Partials 20417 20364 -53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the PR, @jwils We recently did some minor improvements ( #21179 ) to contain this cache size limit, but a rewrite of the cache (like field data cache) is probably what is needed but that's not going to be trivial. We'll need some time to review this PR as there are competing priorities at the moment. |
|
Similar to the other pr. I believe removing the async |
|
Persistent review updated to latest commit 9452a6c |
|
❌ Gradle check result for 9452a6c: TIMEOUT Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| if (keys != null) { | ||
| for (BitsetCacheKey key : keys) { | ||
| cache.invalidate(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we instead do this in a different thread rather than doing in a sync manner? As I worry that this method might get triggered on a cluster applier thread while trying to close a shard(and eventually its readers), and might unnecessarily block it if it takes a lot of time (like we have seen in the past) and causes node drops as a result.
There was a problem hiding this comment.
My preference would be to do it on one thread. I think it simplifies the logic where it doesn't need to be async. I don't think this can block for very long as these invalidate operations should be O(1). I think the max number of keys here is bounded by the number of nested fields so I can't imagine this list to invalidate being large enough to cause any issues.
I don't know the history of unnecessary blocks causing cluster blocks, so let me know if you still prefer this async and I'll make the change.
There was a problem hiding this comment.
I think the max number of keys here is bounded by the number of nested fields so I can't imagine this list to invalidate being large enough to cause any issues.
Oh yeah! I missed the fact that for this cache, we won't have much queries cached per segment reader as it is bounded by the setting index.mapping.nested_fields.limit which is 50. Someone can set higher ofcourse to a very large value as there is no max defined it seems, but that's not a good practice and will break something else. So I agree that we can keep this as a sync operation.
|
Persistent review updated to latest commit 2e255f5 |
|
@andrross @jainankitk Can you folks also review/approve and help in merging it in? |
|
Persistent review updated to latest commit 413decd |
Description
The node-level bitset filter cache introduced in #21179 defers cleanup of entries from closed readers to a periodic sweep over
Cache#keys(). That sweep is unreliable: the livekeys()iterator walks the LRU linked list without holding the LRU lock, and iteration under concurrent mutation is undefined. Every cache hit relinks the accessed entry to the head of the LRU list, so an entry promoted mid-sweep can end up behind the cursor and never be visited. BecausepurgeStaleEntries()unconditionally drainsstaleCacheKeysafter each pass and a reader closes only once, a skipped entry is never revisited: it stays in the cache until size-based eviction reaches it — potentially forever — pinning its bitset, its query, and theValue.listener → IndexServicegraph on heap while being invisible to shard bitset stats.This applies the same fix as the fielddata cache in #22491: invalidate the exact keys synchronously when the reader closes, instead of marking the reader stale and relying on a sweep.
getAndLoadIfNotPresent()records every key cached for a reader in a per-reader registry (keysByReader); the closed listener is registered at most once per reader via the registry's atomiccomputeIfAbsent. Keys are only unregistered on reader close: trimming on eviction would race with a concurrent reload of the same key re-registering it, and a lost registration would leave the entry with no cleanup path. A registered key whose entry was evicted costs two references, is bounded by the reader's lifetime, and invalidating it on close is a no-op.onClose()invalidates exactly the keys registered for the closed reader, synchronously. A reader closes only once and exact-key invalidation is O(#queries cached for the reader), so there is no need to batch it.BitsetCacheCleaner, theindices.cache.bitset.cleanup_intervalsetting,purgeStaleEntries(), and the stale-mark bookkeeping are removed, and the constructor no longer needs aThreadPool.Related Issues
Related to #21179. Companion to #22491, which made the same change for the fielddata cache. No longer depends on #22499 (
Cache#keysSnapshot()): with exact-key invalidation there is no sweep left in this cache.Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.