refactor lookups to be more chill to router #7222
Conversation
|
Oops, test failures is legit, fixing the |
|
Fixed and combined both |
|
Would the module introduced in this PR also be a good solution to the problem of #5727 since it would allow lookup serde without loading lookups? Perhaps I could rename |
…torFactoryContainerProvider to NoopLookupExtractorFactoryContainerProvider
Renamed to |
|
This is interesting. |
Ah, sorry I hadn't got around to that dev list thread yet, I'll have a look and see if I can move some stuff around since I have to fix up conflicts anyway. |
|
@yurmix @drcrallen With this refactor I was able to move these things to the
|
|
Wow, thanks so much! Now I'm going to reinitiate my efforts on multi-value lookups. |
* refactor lookups to be more chill to router * remove accidental change * fix and combine LookupIntrospectionResourceTest * fix inspection * rename RouterLookupModule to LookupSerdeModule and RouterLookupExtractorFactoryContainerProvider to NoopLookupExtractorFactoryContainerProvider * make comment generic * use ConfigResourceFilter instead of StateResourceFilter * fix indentation * unused import * another unused import * refactor some stuff into processing module, split up LookupModule.java classes into their own files
| binder.bindConstant().annotatedWith(Names.named("serviceName")).to("druid/router"); | ||
| binder.bindConstant().annotatedWith(Names.named("servicePort")).to(8888); | ||
| binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(9088); | ||
| binder -> { |
There was a problem hiding this comment.
I'm not sure an anonymous class conversion to a lambda makes code better in this case. You lose visibility into the fact that this lambda is a Module and that the method is configure(), making the code more cryptic, IMO.
This PR introduces
LookupSerdeModulewhich allows the router to perform query serde but without having to load lookups as happens using the stockLookupModulewith default settings. To achieve this, an interface has been extracted fromLookupReferencesManager, the very java namedLookupExtractorFactoryContainerProvider, which is now used everywhereLookupReferencesManagerwas previously used, except forLookupListeningResource. I think this is a bit nicer, because now all query processing stuff no longer has obvious access to methods to load and drop lookups, only the method to get theLookupExtractorFactoryContainerneeded to do the transforms. Additionally, this has allowed many of these types to be migrated intodruid-processingwhich seems like a more appropriate home for them (including the newLookupExtractorFactoryContainerProviderinterface).Now in
druid-processing:LookupDimensionSpecLookupExprMacroLookupExtractorFactoryContainerProviderRegisteredLookupExtractionFnAn integration test has been added to the wiki tests, which loads a simple json lookup, and tests queries against brokers and routers. This also means there is now an integration test that is testing lookups! I've also tested this on a small test cluster spun up on my laptop.
The router will proxy requests to the lookup introspection API it was previously serving, so if the router was being queried to introspect lookups things should still function, it will just be proxied to a default broker now.
I suspect it wasn't too much of an issue, because that API didn't appear functional because didn't have auth checks performed, and would throw an exception of the form
So I've included a fix for that as well.
I also made some minor adjustments to the integration tests configurations, switching gc from cms to g1, and lowering some thread pool sizes to maybe result in a slightly lighter footprint.