-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deguice ActionFilters #26691
Deguice ActionFilters #26691
Conversation
Allows to instantiate TransportAction instances without Guice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
I think it is probably worth passing ThreadPool
as an argument to getActionFilters
.
ResourceWatcherService resourceWatcherService, ScriptService scriptService, | ||
NamedXContentRegistry xContentRegistry, Environment environment, | ||
NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) { | ||
loggingFilter.set(new LoggingFilter(clusterService.getSettings(), threadPool)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a sign that getActionFilters
maybe should take ThreadPool
as an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is meant to be extended by plugins, it's unclear what parameters would be useful to them. I think it's best to keep the interface lean.
ResourceWatcherService resourceWatcherService, ScriptService scriptService, | ||
NamedXContentRegistry xContentRegistry, Environment environment, | ||
NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) { | ||
testFilter.set(new ReindexFromRemoteWithAuthTests.TestFilter(threadPool)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of createComponents is to create services. This creates an unenforcible ordering dependency between creating these services and calling getActionFilters()
. getActionFilters
should take whatever is necessary to creation action filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getActionFilters
is meant to be used by plugins, for which we don't know about the parameters that they require. We have the same situation for a lot of other methods (e.g. getRestHandlers). In case of incorrect ordering, our tests will blow up.
Allows to instantiate TransportAction instances without Guice.
* master: [DOCS] Added index-shared4 and index-shared5.asciidoc BulkProcessor flush runnable preserves the thread context from creation time (elastic#26718) Catch exceptions and inform handler in RemoteClusterConnection#collectNodes (elastic#26725) [Docs] Fix name of character filter in example. (elastic#26724) Remove parse field deprecations in query builders (elastic#26711) elastic#26720: Set the correct bwc version after backport to 6.0 Remove deprecated type and slop field in MatchQueryBuilder (elastic#26720) Refactoring of Gateway*** classes (elastic#26706) Make RestHighLevelClient's Request class public (elastic#26627) Deguice ActionFilter (elastic#26691) aggs: Allow aggregation sorting via nested aggregation. Build: Set bwc builds to always set snapshot (elastic#26704) File Discovery: Remove fallback with zen discovery (elastic#26667)
This makes it possible to instantiate TransportAction (sub)-classes without Guice.