You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/11/01 05:13:37 UTC
[GitHub] [pinot] vvivekiyer opened a new pull request, #9697: AdaptiveServerSelection: Fix timer
vvivekiyer opened a new pull request, #9697:
URL: https://github.com/apache/pinot/pull/9697
Label = BugFix
`ExponentialWeightedMovingAverage` class currently uses a `Timer` to 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.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #9697: AdaptiveServerSelection: Fix timer
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #9697:
URL: https://github.com/apache/pinot/pull/9697#discussion_r1010909353
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/server/routing/stats/ServerRoutingStatsManager.java:
##########
@@ -47,8 +48,13 @@ public class ServerRoutingStatsManager {
private final PinotConfiguration _config;
private volatile boolean _isEnabled;
private ConcurrentHashMap<String, ServerRoutingStatsEntry> _serverQueryStatsMap;
+
+ // Main executor service for collecting and aggregating stats for all servers.
private ExecutorService _executorService;
+ // ScheduledExecutorServer for processing periodic tasks like decay.
+ private ScheduledExecutorService _periodicTaskExecutor;
Review 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.
Thoughts?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on pull request #9697: AdaptiveServerSelection: Fix timer
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on PR #9697:
URL: https://github.com/apache/pinot/pull/9697#issuecomment-1298800876
@dmyang please review.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #9697: AdaptiveServerSelection: Fix timer
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #9697:
URL: https://github.com/apache/pinot/pull/9697#discussion_r1010909353
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/server/routing/stats/ServerRoutingStatsManager.java:
##########
@@ -47,8 +48,13 @@ public class ServerRoutingStatsManager {
private final PinotConfiguration _config;
private volatile boolean _isEnabled;
private ConcurrentHashMap<String, ServerRoutingStatsEntry> _serverQueryStatsMap;
+
+ // Main executor service for collecting and aggregating stats for all servers.
private ExecutorService _executorService;
+ // ScheduledExecutorServer for processing periodic tasks like decay.
+ private ScheduledExecutorService _periodicTaskExecutor;
Review 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?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia merged pull request #9697: AdaptiveServerSelection: Fix timer
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #9697:
URL: https://github.com/apache/pinot/pull/9697
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] dmyang commented on a diff in pull request #9697: AdaptiveServerSelection: Fix timer
Posted by GitBox <gi...@apache.org>.
dmyang commented on code in PR #9697:
URL: https://github.com/apache/pinot/pull/9697#discussion_r1010913789
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/server/routing/stats/ServerRoutingStatsManager.java:
##########
@@ -47,8 +48,13 @@ public class ServerRoutingStatsManager {
private final PinotConfiguration _config;
private volatile boolean _isEnabled;
private ConcurrentHashMap<String, ServerRoutingStatsEntry> _serverQueryStatsMap;
+
+ // Main executor service for collecting and aggregating stats for all servers.
private ExecutorService _executorService;
+ // ScheduledExecutorServer for processing periodic tasks like decay.
+ private ScheduledExecutorService _periodicTaskExecutor;
Review Comment:
👍 Thanks for explaining reasoning. That makes sense to me and seems fine to leave as is.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] dmyang commented on a diff in pull request #9697: AdaptiveServerSelection: Fix timer
Posted by GitBox <gi...@apache.org>.
dmyang commented on code in PR #9697:
URL: https://github.com/apache/pinot/pull/9697#discussion_r1010890179
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/server/routing/stats/ServerRoutingStatsManager.java:
##########
@@ -47,8 +48,13 @@ public class ServerRoutingStatsManager {
private final PinotConfiguration _config;
private volatile boolean _isEnabled;
private ConcurrentHashMap<String, ServerRoutingStatsEntry> _serverQueryStatsMap;
+
+ // Main executor service for collecting and aggregating stats for all servers.
private ExecutorService _executorService;
+ // ScheduledExecutorServer for processing periodic tasks like decay.
+ private ScheduledExecutorService _periodicTaskExecutor;
Review 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
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org