jihoonson commented on a change in pull request #6094: Introduce SystemSchema
tables (#5989)
URL: https://github.com/apache/incubator-druid/pull/6094#discussion_r223201354
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java
##########
@@ -57,36 +61,52 @@
.build();
private final DruidSchema druidSchema;
+ private final TimelineServerView serverView;
private final QueryLifecycleFactory queryLifecycleFactory;
private final DruidOperatorTable operatorTable;
private final ExprMacroTable macroTable;
private final PlannerConfig plannerConfig;
private final ObjectMapper jsonMapper;
private final AuthorizerMapper authorizerMapper;
+ private final DruidLeaderClient coordinatorDruidLeaderClient;
+ private final DruidLeaderClient overlordDruidLeaderClient;
@Inject
public PlannerFactory(
final DruidSchema druidSchema,
+ final TimelineServerView serverView,
final QueryLifecycleFactory queryLifecycleFactory,
final DruidOperatorTable operatorTable,
final ExprMacroTable macroTable,
final PlannerConfig plannerConfig,
final AuthorizerMapper authorizerMapper,
- final @Json ObjectMapper jsonMapper
+ final @Json ObjectMapper jsonMapper,
+ final @Coordinator DruidLeaderClient coordinatorDruidLeaderClient,
+ final @IndexingService DruidLeaderClient overlordDruidLeaderClient
Review comment:
Regarding my comment about mocking `DruidLeaderClient` in benchmark, I think
the real issue is that `PlannerFactory` needs `TimelineServerView`,
`ObjectMapper`, and `DruidLeaderClient`s. Since this class is just a factory
which is responsible for creating a new planner, it's not intuitive why it gets
information from TimelineServerview or leaderClients. This makes me confused
until tracking the whole change related to those parameters.
I think the same design for `DruidSchema` can be applied to here too.
`DruidSchema` also needs `TimelineServerView`, but it's being injected and so
`TimelineServerView` is not exposed here. Similarly, you can inject
`SystemSchema` instead of these parameters. Actually, you've already made it
injectable. Why don't you use it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
