[ML] Implement CCMCache #137743
[ML] Implement CCMCache #137743
Conversation
Implements a lazy-loading cache in front of the CCM Inference index. By default, the cache holds a single entry for 15 minutes, and cache misses search the CCM index and load the responses into the cache. Some design decisions: - The cache maintains an "empty" entry so that the `isPresent` call can reuse the "empty" response to quickly return false. Invalidating the cache or calling get will drop this "empty" entry. - Invalidating the cache will broadcast a message to all nodes so that all caches on all nodes will invalidate their caches. - Since the broadcast message only works if all nodes are on the latest version, there is a new NodeFeature to enable the cache once all nodes in the cluster have upgraded.
prwhelan
added
>non-issue
:ml
|
Pinging @elastic/ml-core (Team:ML) |
jonathan-buttner
left a comment
jonathan-buttner
left a comment
There was a problem hiding this comment.
Thanks Pat! Left a few suggestions.
| return state.clusterRecovered() && featureService.clusterHasFeature(state, InferenceFeatures.INFERENCE_CCM_CACHE); | ||
| } | ||
|
|
||
| private void enabled(ProjectId projectId, CCMModel ccmModel) { |
There was a problem hiding this comment.
nit: It was a bit hard to understand enabled/disabled, cacheEnabled, isEnabled, and CCMModelEntry::enabled. What about for the methods that do the put we call them enabledEntry(), setEnabled, or createEnabledEntry?
| this.client = client; | ||
| } | ||
|
|
||
| public void get(ActionListener<CCMModel> listener) { |
There was a problem hiding this comment.
Could you add a comment mentioning that the expectation is that a caller would first interact with isEnabled before calling get() and mention that if the configuration entry is in the disabled state that this call with read through to the backing index.
|
|
||
| public class CCMCacheTests extends ESSingleNodeTestCase { | ||
|
|
||
| private static final TimeValue TIMEOUT = new TimeValue(30, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Instead of using this could we use TimeValue.THIRTY_SECONDS?
| assertFalse(isPresent()); | ||
| assertThat(ccmCache.stats().getHits(), equalTo(1L)); | ||
| assertThat(ccmCache.stats().getMisses(), equalTo(1L)); | ||
| } |
There was a problem hiding this comment.
I might have missed it but do we have a test for the else case in this block:
CCMCache::get
if (cachedEntry != null && cachedEntry.enabled()) {
listener.onResponse(cachedEntry.ccmModel());
} else {
Could we add a test that when the internal entry is in the disabled state (but present and not null) that we get a cache miss aka hit the else case?
3 participants
Implements a lazy-loading cache in front of the CCM Inference index. By default, the cache holds a single entry for 15 minutes, and cache misses search the CCM index and load the responses into the cache.
Some design decisions:
isPresentcall can reuse the "empty" response to quickly return false. Invalidating the cache or calling get will drop this "empty" entry.