Allow a plugin to supply its own query cache implementation #12881
Conversation
| bind(QueryCache.class).to(queryCacheImpl).in(Scopes.SINGLETON); | ||
| } | ||
|
|
||
| public void setQueryCacheClass(Class<? extends QueryCache> queryCacheClass) { |
There was a problem hiding this comment.
Can we do a named implementation instead? See the ExtensionPoint class that was added recently. I think these should be named (as they are now with "index" and "none") and registered, and then set with the "index.queries.cache.type" setting as the others are.
There was a problem hiding this comment.
tricky.. if a plugin needs to wrap the cache to apply certain cache behavior regardless of the specific cache impl, named caches are not enough. I do like idea of having an extensible named cache though, so maybe we can have both? maybe introduce the notion of a cache wrapper?
There was a problem hiding this comment.
The plugin can set additionalSettings that specify the cache impl it wants to force.
There was a problem hiding this comment.
sure, but with the recent changes, it won't be able to do that if the setting already exists
There was a problem hiding this comment.
It still can. The plugin just needs to enforce that the user settings cannot contain that setting (so that the additional settings are not overriden by the user settings). This can be done by throwing an exception on init.
There was a problem hiding this comment.
that will prevent user defined cache implementation + cache wrapping. We should still enable custom query implementations while enable plugins to enforce things like making certain queries "non-cacheable"
There was a problem hiding this comment.
Except the current implementation I see for these type of things does a complete override, not wrapping (and how would you wrap a class for binding? you need an instance?). If we want to allow custom filtering on top of setting a custom cache, then we should have the necessary extension points to customize or configure the cache for this behavior.
martijnvg
force-pushed
the
allow_for_customable_query_cache
branch
from
de01985
to
b4526cc
Compare
|
@uboness @rjernst I've updated the PR use I think the fact that a plugin can fail initialization if another query cache implementation has been configured via node/index settings than the implementation a plugin wishes to use, is sufficient for now. |
|
Looks good, but can you add a test? See for example SearchModuleTests. Check a custom one can be registered and set, and that registering a duplicate name fails. |
|
@rjernst I've added a test. |
|
LGTM, thanks! |
martijnvg
force-pushed
the
allow_for_customable_query_cache
branch
from
1be6acb
to
12c40fa
Compare
|
LGTM 2 |
…ache Allow a plugin to supply its own query cache implementation
…ache Allow a plugin to supply its own query cache implementation
No description provided.