imply-cheddar commented on code in PR #17572:
URL: https://github.com/apache/druid/pull/17572#discussion_r1887170664
##########
server/src/main/java/org/apache/druid/guice/DruidBinders.java:
##########
@@ -62,32 +66,68 @@ public static MapBinder<Class<? extends Query>, QueryLogic>
queryLogicBinderType
);
}
- public static QueryLogicBinder queryLogicBinder(Binder binder)
+ public static QueryBinder queryBinder(Binder binder)
{
- return new QueryLogicBinder(binder);
+ return new QueryBinder(binder);
}
- public static class QueryLogicBinder
+ public static class QueryBinder
Review Comment:
This class is hiding things from people if they ever need them. I'm not
sure if it will ever be needed, but it's relatively easy for this class to
expose things that allow someone to do whatever they need while still
maintaining the simplicity for the common case.
##########
sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java:
##########
@@ -896,13 +933,12 @@ private SqlTestFramework(Builder builder)
// test pulls in a module, then pull in that module, even though we are
// not the Druid node to which the module is scoped.
.ignoreLoadScopes();
- List<Module> overrideModules = new ArrayList<>(builder.overrideModules);
- overrideModules.add(new LookylooModule());
- overrideModules.add(new SqlAggregationModule());
- overrideModules.add(new SegmentWranglerModule());
- overrideModules.add(new ExpressionModule());
+ ArrayList<Module> overrideModules = new
ArrayList<>(builder.overrideModules);
+
+ injectorBuilder.add(componentSupplier.getCoreModule());
overrideModules.add(testSetupModule());
+ overrideModules.add(componentSupplier.getOverrideModule());
Review Comment:
The base framework should not add anything to override modules. Override
modules exist to allow the *tests* to override things that the framework setup.
If the framework adds stuff to overrides here, then the tests cannot override
those things, causing problems when tests need to change behaviors. The
override modules need to be wholly owned and controlled by the test setup. The
fact that these modules are being added as overrides points to the framework
not being setup properly. If it is running into competing bindings, it needs
to resolve them by either overriding the competing module when it's added *or*
by applying these overrides in a way that allows for further override by the
tests.
##########
server/src/main/java/org/apache/druid/guice/DruidBinders.java:
##########
@@ -62,32 +66,68 @@ public static MapBinder<Class<? extends Query>, QueryLogic>
queryLogicBinderType
);
}
- public static QueryLogicBinder queryLogicBinder(Binder binder)
+ public static QueryBinder queryBinder(Binder binder)
{
- return new QueryLogicBinder(binder);
+ return new QueryBinder(binder);
}
- public static class QueryLogicBinder
+ public static class QueryBinder
{
- private MapBinder<Class<? extends Query>, QueryLogic> queryLogicMapBinder;
- private Binder binder;
+ MapBinderHelper<Class<? extends Query>, QueryLogic> queryLogicBinder;
+ MapBinderHelper<Class<? extends Query>, QueryRunnerFactory>
queryRunnerFactoryBinder;
+ MapBinderHelper<Class<? extends Query>, QueryToolChest>
queryToolChestBinder;
+
- public QueryLogicBinder(Binder binder)
+ public QueryBinder(Binder binder)
{
- this.binder = binder;
- queryLogicMapBinder = DruidBinders.queryLogicBinderType(binder);
+ queryLogicBinder = new MapBinderHelper<>(
+ binder, queryLogicBinderType(binder),
ImmutableSet.of(LazySingleton.class)
Review Comment:
Taking the list of scopes is not really helpful here. There should just be
a `naiveBinding` method for each of the types of things that forces into a
`LazySingleton`. If someone wants something other than that, you need to make
both the `MapBinder` and the `Binder` available to them through getters.
Exposing the MapBinder and Binder makes this object a true helper because it
doesn't inhibit users from doing what they need. If you hide either of those
objects, you are now artificially hiding parts of Guice from someone who uses
this object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
