Jackie-Jiang commented on code in PR #16080:
URL: https://github.com/apache/pinot/pull/16080#discussion_r2145913946
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageQueryThrottler.java:
##########
@@ -56,26 +57,28 @@ public class MultiStageQueryThrottler implements
ClusterChangeHandler {
private HelixConfigScope _helixConfigScope;
private int _numBrokers;
private int _numServers;
+ private AdjustableSemaphore _semaphore;
/**
* If _maxServerQueryThreads is <= 0, it means that the cluster is not
configured to limit the number of multi-stage
* queries that can be executed concurrently. In this case, we should not
block the query.
*/
private int _maxServerQueryThreads;
- private AdjustableSemaphore _semaphore;
+ private final int _maxServerQueryThreadsFromBrokerConfig;
private final AtomicInteger _currentQueryServerThreads = new AtomicInteger();
Review Comment:
(minor, convention) Put `final` variables in front of others
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageQueryThrottler.java:
##########
@@ -86,7 +89,11 @@ public void init(HelixManager helixManager) {
.count());
if (_maxServerQueryThreads > 0) {
- _semaphore = new AdjustableSemaphore(Math.max(1, _maxServerQueryThreads
* _numServers / _numBrokers), true);
+ int semaphoreLimit = Math.max(1, _maxServerQueryThreads * _numServers /
_numBrokers);
+ LOGGER.info("Setting estimated server query threads limit: {} for
maxServerQueryThreads: {}, "
+ + "numBrokers: {}, and numServers: {}",
+ semaphoreLimit, _maxServerQueryThreads, _numBrokers, _numServers);
+ _semaphore = new AdjustableSemaphore(semaphoreLimit, true);
Review Comment:
(minor) Consider extracting this into a helper method
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -46,21 +50,31 @@ public HardLimitExecutor(int max, ExecutorService
executorService) {
*/
public static int getMultiStageExecutorHardLimit(PinotConfiguration config) {
try {
- int maxThreads = Integer.parseInt(config.getProperty(
+ int maxThreadsFromClusterConfig = Integer.parseInt(config.getProperty(
Review Comment:
The passed in `PinotConfiguration` is server config. We allow setting it as
cluster config because all cluster config will be added into server config if
it is not already configured.
Here we are essentially adding a new config which is an absolute value,
instead of a factor
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -46,21 +50,26 @@ public HardLimitExecutor(int max, ExecutorService
executorService) {
*/
public static int getMultiStageExecutorHardLimit(PinotConfiguration config) {
try {
- int maxThreads = Integer.parseInt(config.getProperty(
+ int serverConfigLimit =
config.getProperty(CommonConstants.Server.CONFIG_OF_MSE_MAX_EXECUTION_THREADS,
+ CommonConstants.Server.DEFAULT_MSE_MAX_EXECUTION_THREADS);
+ if (serverConfigLimit > 0) {
+ return serverConfigLimit;
+ }
+ int maxThreadsFromClusterConfig = Integer.parseInt(config.getProperty(
CommonConstants.Helix.CONFIG_OF_MULTI_STAGE_ENGINE_MAX_SERVER_QUERY_THREADS,
CommonConstants.Helix.DEFAULT_MULTI_STAGE_ENGINE_MAX_SERVER_QUERY_THREADS
));
int hardLimitFactor = Integer.parseInt(config.getProperty(
CommonConstants.Helix.CONFIG_OF_MULTI_STAGE_ENGINE_MAX_SERVER_QUERY_HARDLIMIT_FACTOR,
CommonConstants.Helix.DEFAULT_MULTI_STAGE_ENGINE_MAX_SERVER_QUERY_HARDLIMIT_FACTOR
));
- if (maxThreads <= 0 || hardLimitFactor <= 0) {
+ if (maxThreadsFromClusterConfig <= 0 || hardLimitFactor <= 0) {
return 0;
}
- return maxThreads * hardLimitFactor;
+ return maxThreadsFromClusterConfig * hardLimitFactor;
} catch (NumberFormatException e) {
- return
Integer.parseInt(CommonConstants.Helix.DEFAULT_MULTI_STAGE_ENGINE_MAX_SERVER_QUERY_THREADS)
- *
Integer.parseInt(CommonConstants.Helix.DEFAULT_MULTI_STAGE_ENGINE_MAX_SERVER_QUERY_HARDLIMIT_FACTOR);
+ LOGGER.warn("Failed to parse multi-stage executor hard limit from
config. Hard limiting will be disabled.", e);
+ return -1;
Review Comment:
(minor) `return 0` to be consistent with line 67
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
