kgyrtkirk commented on code in PR #17775:
URL: https://github.com/apache/druid/pull/17775#discussion_r1984684824
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java:
##########
@@ -91,6 +94,10 @@ public SegmentLoadDropHandler(
Executors.newScheduledThreadPool(
config.getNumLoadingThreads(),
Execs.makeThreadFactory("SimpleDataSegmentChangeHandler-%s")
+ ),
+ Executors.newScheduledThreadPool(
+ config.getNumBootstrapThreads(),
Review Comment:
note: this will create a threadpool with a corepool of
`getNumBootstrapThreads` ; if those threads are not doing anything it will just
hold on to the stack memory that needs
I wonder if this feature does need a separate executor - does that really
have to be running with non-zero corepoolsize all the time?
##########
server/src/main/java/org/apache/druid/server/coordination/SegmentLoadDropHandler.java:
##########
@@ -91,6 +94,10 @@ public SegmentLoadDropHandler(
Executors.newScheduledThreadPool(
config.getNumLoadingThreads(),
Execs.makeThreadFactory("SimpleDataSegmentChangeHandler-%s")
Review Comment:
its unclear to me why the need for a second executor pool; it would be
possibly better to use a more aggressively tuned threadpool first and then go
back to using a more conservative one.
The usage of `Executors.newScheduledThreadPool` is kinda unfortunate as it
will retain all these threads forever; a `ThreadPoolExecutor` with
`allowCoreThreadTimeOut` could go back to `0` threads if not in use
--
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]
