-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Enhance injector usage during conglomerate builds in tests #17331
base: master
Are you sure you want to change the base?
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
TestBufferPool
and other things more easily