fix(kvblock): early-stop lookup on missing key or empty filtered pods - #2713
fix(kvblock): early-stop lookup on missing key or empty filtered pods#2713quangh33 wants to merge 4 commits into
Conversation
github-actions
Bot
added
area/kvcache
size/L
github-actions
Bot
added
kind/bug
|
I found one telemetry issue: early termination changes |
There was a problem hiding this comment.
🟡 Changes recommended
The updated cost-aware size test asserts a specific eviction outcome for a Ristretto-backed cache, which is not guaranteed and risks test flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns the in-memory KV-block index backends (InMemoryIndex and CostAwareMemoryIndex) with the prefix-chain semantics already used by RedisIndex.Lookup, ensuring lookups early-stop when the prefix breaks (missing key or filter produces no matching pods).
Changes:
- Early-return from
Lookupon missing keys and on empty filtered pod results inInMemoryIndexandCostAwareMemoryIndex. - Add common behavior tests to enforce early-stop semantics across all
Indeximplementations. - Update in-memory and cost-aware size/eviction tests to avoid multi-key lookups that violate prefix lookup semantics.
File summaries
| File | Description |
|---|---|
| pkg/kvcache/kvblock/index_test.go | Adds common tests for early-stop behavior on misses and filter-empty results. |
| pkg/kvcache/kvblock/in_memory.go | Implements early-stop on missing key and empty filtered pod list during prefix lookup. |
| pkg/kvcache/kvblock/in_memory_test.go | Adjusts eviction test to query evicted and remaining keys in separate lookups. |
| pkg/kvcache/kvblock/cost_aware_memory.go | Implements early-stop on missing key and empty filtered pod list during prefix lookup. |
| pkg/kvcache/kvblock/cost_aware_memory_test.go | Updates eviction test to use separate lookups consistent with prefix semantics. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
@yankay I added TestScoreTokensBlockHitTelemetryEarlyTermination in pkg/kvcache/indexer_trace_test.go to verify the tememetry. |
In-memory and cost-aware memory index implementations continued traversing subsequent request keys when a key was not found or when filtering by pod identifiers yielded no matches. Because KV block lookup represents a contiguous prefix match, breaking the prefix chain invalidates subsequent keys and must terminate the search immediately, matching RedisIndex. Signed-off-by: quangh33 <quanghd.95vn@gmail.com>
…rly termination Early termination in Index.Lookup cuts the block search at the first missing key. Verify that keys after the miss do not count towards blocks_found and block_hit_ratio for a [hit, miss, hit] sequence. Signed-off-by: quangh33 <quanghd.95vn@gmail.com>
CostAwareMemoryIndex uses Ristretto TinyLFU admission and sampled eviction, which can reject incoming keys rather than strictly evicting earlier keys in LRU order. Assert total retained key count instead of specific key presence. Signed-off-by: quangh33 <quanghd.95vn@gmail.com>
f1522da to
abefe4a
Compare
| traceLogger.Info("no pods found for key, cutting search", "key", requestKey) | ||
| } | ||
| continue | ||
| return podsPerKey, nil // early stop since prefix-chain breaks here |
There was a problem hiding this comment.
Thanks for rebasing this. The new missing-key return bypasses the final ctx.Err() check. Cancellation after the last checkpoint can therefore return partial results with nil, while the filtered-empty path below returns the cancellation error. Could we use the same check here and cover cancellation between a checkpoint and a miss?
What type of PR is this?
/kind bug
What this PR does / why we need it:
In-memory and cost-aware memory index implementations (
InMemoryIndexandCostAwareMemoryIndex) did not early-stop when a key inrequestKeyswas not found (!found) or when filtering againstpodIdentifierSetresulted in an empty pod list. Instead, they logged a message and continued traversing remaining keys. This is inconsistent with the behavior inRedisIndex.Because
Index.Lookuprepresents a contiguous prefix-cache lookup (longest prefix match), breaking the prefix chain at any key invalidates subsequent keys and must terminate traversal immediately, matching the behavior ofRedisIndex.This PR:
return podsPerKey, nil) on missing keys and empty filtered pod slices inInMemoryIndex.LookupandCostAwareMemoryIndex.Lookup.EarlyStopOnMissandEarlyStopOnFilteredEmptytest cases totestCommonIndexBehaviorinindex_test.goto enforce prefix-break early-stop semantics across allIndexbackends.TestInMemoryIndexSizeandTestCostAwareIndexSizeto query evicted keys and remaining cached keys independently, respecting prefix lookup semantics.Which issue(s) this PR fixes:
Fixes #
Release note (write
NONEif no user-facing change):NONE