AdaptiveServerSelection: Fix timer #9697
Conversation
vvivekiyer
force-pushed
the
serverSelectionTimer
branch
from
b8031f8
to
e727fa5
Compare
|
@dmyang please review. |
| private ExecutorService _executorService; | ||
|
|
||
| // ScheduledExecutorServer for processing periodic tasks like decay. | ||
| private ScheduledExecutorService _periodicTaskExecutor; |
There was a problem hiding this comment.
Could the periodic tasks share the same executor service by changing the main executor service to a scheduled one? That way the threadpool size can still be modified w/ pinot config
There was a problem hiding this comment.
The reasoning for not using the same executorService is as follows: The main executor service is responsible for collecting stats about servers. The number of periodic tasks is proportional to the number of servers. And we wouldn't want it to interfere with stats collection - which is critical for making the routing decision.
However, we can have a separate config to control the number of periodic task threads. But I do not see us needing more than 1 thread for this purpose. Let's say, autoDecay window is 30seconds. With 200 severs, we would be generating 2*200 = 400 periodic tasks every 30 seconds. A single thread should be sufficient to process these tasks.
I've also DM'ed you on the Apache Pinot slack channel.
Thoughts?
There was a problem hiding this comment.
👍 Thanks for explaining reasoning. That makes sense to me and seems fine to leave as is.
Label = BugFix
ExponentialWeightedMovingAverageclass currently uses aTimerto periodically autoDecay the value if there are no updates. We maintain stats for each Server. So if there number of servers are too many, the number JVM threads could blow up.In this PR we create a ScheduledThreadPoolExecutor with just a single thread. This thread is responsible for processing all the periodic tasks.