adarshsanjeev commented on code in PR #17775:
URL: https://github.com/apache/druid/pull/17775#discussion_r2010031514
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java:
##########
@@ -88,9 +93,17 @@ public SegmentLoadDropHandler(
config,
announcer,
segmentManager,
- Executors.newScheduledThreadPool(
+ new ScheduledThreadPoolExecutor(
config.getNumLoadingThreads(),
- Execs.makeThreadFactory("SimpleDataSegmentChangeHandler-%s")
+ Execs.makeThreadFactory("SegmentLoadDropHandler-normal-%s")
+ ),
+ new ThreadPoolExecutor(
+ config.getNumBootstrapThreads(),
Review Comment:
Looked into this, I believe that the correct configuration would be to keep
the original approach and have `getNumBootstrapThreads` as the core threadpool
size. The difference is that the executor creates new threads for tasks up to
the core size, as opposed to enqueuing them. In our case, we would want a task
for each segment load, and hence the initial code seems the most correct. I
have added a comment here.
I have also moved the function back from Execs to this class, as adding the
timeout only makes sense if with the core threads can timeout. I don't think
this would be reused much. Even the `Executors` class does not have this as a
function and recommends calling the constructor directly for these cases.
We can discuss this further in this thread if this is not the best approach,
and I can make the changes.
--
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]
