Enhance injector usage during conglomerate builds in tests #17331
Conversation
|
I was working on something else - seen that this might be needed; but right now it may not be required - however the whole conglomerate should be constructed in an injector aware way later...to give better control/etc |
kgyrtkirk
force-pushed
the
createconglomerate-inject1
branch
from
150477e
to
5bc29e3
Compare
This reverts commit b494861.
| default <T, QueryType extends Query<T>> QueryToolChest<T, QueryType> getToolChest(QueryType query) | ||
| { | ||
| QueryRunnerFactory<T, QueryType> factory = findFactory(query); | ||
| if (factory == null) { | ||
| throw DruidException | ||
| .defensive("QueryRunnerFactory for QueryType [%s] is not registered!", query.getClass().getName()); | ||
| } | ||
| return factory.getToolchest(); | ||
| } |
There was a problem hiding this comment.
Originally, the Warehouse and the Conglomerate were separated because the conglomerate was only needed on "leaf nodes". When we introduced the idea of sub-queries, it made it true that brokers also need the conglomerate, which takes away the reason for having the objects separated. I don't think this needs to have a default implementation, but instead we should just make the conglomerate be the only thing depended on. The Warehouse should be deprecated, but continue to exist for a bit (one release) just to enable any extensions that depend on it to be updated to depend on the conglomerate instead.
|
|
||
| package org.apache.druid.query; | ||
|
|
||
| public class ConglomerateBackedQueryToolChestWarehouse implements QueryToolChestWarehouse |
There was a problem hiding this comment.
Don't think you need this, can just update the DefaultQueryRunnerFactoryConglomerate to depend on the same map that the MapQueryToolChestWarehouse and then get rid of this class.
TestBufferPooland other things more easily