kfaraz commented on code in PR #16887:
URL: https://github.com/apache/druid/pull/16887#discussion_r1722743485
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -205,7 +223,67 @@ public void configure(Properties properties)
protected List<? extends Module> getModules()
{
return ImmutableList.of(
- new DruidProcessingModule(),
+ Modules.override(new DruidProcessingModule()).with(
+ new Module()
+ {
+ @Override
+ public void configure(Binder binder)
+ {
+
+ }
+
+ @Provides
+ @ManageLifecycle
+ public QueryProcessingPool getProcessingExecutorPool(
+ Task task,
+ DruidProcessingConfig config,
+ ExecutorServiceMonitor executorServiceMonitor,
+ Lifecycle lifecycle
+ )
+ {
+ if (!task.supportsQueries()) {
+ return new ForwardingQueryProcessingPool(Execs.dummy());
+ }
+ return new MetricsEmittingQueryProcessingPool(
+ PrioritizedExecutorService.create(
+ lifecycle,
+ config
+ ),
+ executorServiceMonitor
+ );
Review Comment:
> The ForwardingQueryProcessingPool(Execs.dummy()) would do exactly that
unless I am mistaken. The task would be delegated to the [dummy
executor](https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/java/util/common/concurrent/DummyExecutorService.java#L35-L35)
which throws UOE on any attempt to submit the task.
While this is true, there are small differences in using a dedicated
`NoopQueryProcessingPool`:
- The intent is clearer to someone reading the code. Using the `Noop`
implementation implies that it is meant to do nothing. Using a `Forwarding`
pool with a dummy executor could mean that it is supposed to have partial
functionality.
- The error message (and perhaps the stack trace too) would be more
user-friendly. When using `Noop` pool, the exception is thrown by the
processing pool itself rather than the underlying dummy executor service.
That said, this is not a blocker for this PR as it is a style choice really.
There are already some quirks of the `QueryProcessingPool` interface that
could use some cleanup. We could address this then.
--
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]
