You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "MeihanLi (via GitHub)" <gi...@apache.org> on 2023/04/18 21:33:30 UTC
[GitHub] [pinot] MeihanLi opened a new pull request, #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
MeihanLi opened a new pull request, #10614:
URL: https://github.com/apache/pinot/pull/10614
### [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Currently Broker uses Jersey default unbounded thread pool to process async requests and it uses No-Op RejectedExecutionHandler.
When a broker is serving very high QPS, a significant amount of threads will be accumulated on the broker. We have seen ~25k threads accumulated in a short time because of this and brokers were taken down very quickly. This is also the root cause of #9019.
This PR
(1) Implements a custom provider `BrokerManagedAsyncExecutorProvider` by extending `ThreadPoolExecutorProvider`, which will be automatically picked by Jersey. When async requests can not be accommodated, they will get rejected and customers will receive ERROR code 503 (Service Unavailable).
(2) Add new configuration options below which allow us to use the bounded thread pool and allocate capacities for it. By default, it is disabled.
`pinot.broker.enable.bounded.http.async.executor`
`pinot.broker.http.async.executor.max.pool.size`
`pinot.broker.http.async.executor.core.pool.size`
`pinot.broker.http.async.executor.queue.size`
By enabling the new configs, endpoints POST /query/sql and GET /query/sql will be impacted.
### Test
Tested locally, one example response for a rejected request is
```
HTTP/1.1 503 Service Unavailable
Content-Type: application/json
Connection: close
Content-Length: 220
Pinot Broker thread pool can not accommodate more requests now. Request is rejected from java.util.concurrent.ThreadPoolExecutor@79c8b52f[Running, pool size = 1, active threads = 1, queued tasks = 1, completed tasks = 6]
```
The error log emitted by BrokerManagedAsyncExecutorProvider is:
`2023/04/14 16:47:26.146 ERROR [BrokerManagedAsyncExecutorProvider] [grizzly-http-server-15] Task java.util.concurrent.FutureTask@dafa6aa[Not completed, task = java.util.concurrent.Executors$RunnableAdapter@74af7b05[Wrapped task = org.glassfish.jersey.server.ServerRuntime$AsyncResponder$2@58e4de25]] rejected from java.util.concurrent.ThreadPoolExecutor@1bebdf0b[Running, pool size = 1, active threads = 1, queued tasks = 1, completed tasks = 6]`
--
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] jasperjiaguo commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1169332961
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
+ "pinot.broker.enable.bounded.http.async.executor";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ "pinot.broker.http.async.executor.max.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ "pinot.broker.http.async.executor.core.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
+ "pinot.broker.http.async.executor.queue.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
Review Comment:
Would the default queue size of availableProcessors() * 2 be too small? During spiky request we may drop requests prematurely?
--
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] Jackie-Jiang merged pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10614:
URL: https://github.com/apache/pinot/pull/10614
--
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] Jackie-Jiang commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1169332617
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -32,6 +32,8 @@ public enum BrokerMeter implements AbstractMetrics.Meter {
QUERIES("queries", false),
// These metrics track the exceptions caught during query execution in broker side.
+ // Query rejected by Jersey thread pool
+ QUERY_REJECTION_EXCEPTIONS("exceptions", true),
Review Comment:
```suggestion
QUERY_REJECTED_EXCEPTIONS("exceptions", true),
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerManagedAsyncExecutorProvider.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import javax.ws.rs.ServiceUnavailableException;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.common.metrics.BrokerMeter;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.glassfish.jersey.server.ManagedAsyncExecutor;
+import org.glassfish.jersey.spi.ThreadPoolExecutorProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * BrokerManagedAsyncExecutorProvider provides a bounded thread pool.
+ */
+@ManagedAsyncExecutor
+public class BrokerManagedAsyncExecutorProvider extends ThreadPoolExecutorProvider {
Review Comment:
Please reformat the changes with [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide)
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
Review Comment:
I don't think it always associates with `http`. To make it specific, shall we name it `jersey.threadpool.executor`?
--
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] jasperjiaguo commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1171847444
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
+ "pinot.broker.enable.bounded.http.async.executor";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ "pinot.broker.http.async.executor.max.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ "pinot.broker.http.async.executor.core.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
+ "pinot.broker.http.async.executor.queue.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
Review Comment:
Yes that looks good to me. Thanks!
--
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] MeihanLi commented on pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #10614:
URL: https://github.com/apache/pinot/pull/10614#issuecomment-1513819210
> LGTM otherwise
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167234729
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,20 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_THREAD_POOL = "pinot.broker.enable.bounded.threadPool";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_THREAD_POOL = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_MAX_POOL_SIZE = "pinot.broker.max.pool.size";
Review Comment:
Thanks Ankit, addressed
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167636949
##########
pinot-broker/src/test/java/org/apache/pinot/broker/broker/BrokerManagedAsyncExecutorProviderTest.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+import java.util.Collections;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.ws.rs.ServiceUnavailableException;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.spi.metrics.PinotMetricUtils;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+
+public class BrokerManagedAsyncExecutorProviderTest {
+
+ public static BrokerMetrics _brokerMetrics;
+
+ @BeforeClass
+ public void setUp() {
+ _brokerMetrics = new BrokerMetrics(
+ CommonConstants.Broker.DEFAULT_METRICS_NAME_PREFIX,
+ PinotMetricUtils.getPinotMetricsRegistry(null),
+ CommonConstants.Broker.DEFAULT_ENABLE_TABLE_LEVEL_METRICS,
+ Collections.emptyList());
+ }
+
+ @Test
+ public void testExecutorService() throws InterruptedException, ExecutionException {
+ // create a new instance of the executor provider
+ BrokerManagedAsyncExecutorProvider provider = new BrokerManagedAsyncExecutorProvider(_brokerMetrics, 2, 2, 2);
+
+ // get the executor service
+ ThreadPoolExecutor executor = (ThreadPoolExecutor) provider.getExecutorService();
+
+ // submit a task to the executor service and wait for it to complete
+ Future<Integer> futureResult = executor.submit(() -> 1 + 1);
+ Integer result = futureResult.get();
+
+ // verify that the task was executed and returned the expected result
+ assertNotNull(result);
+ assertEquals((int) result, 2);
+
+ // wait for the executor service to shutdown
+ executor.shutdown();
+ executor.awaitTermination(1, TimeUnit.SECONDS);
+ }
+
+ @Test
+ public void testGet() {
+ BrokerManagedAsyncExecutorProvider provider = new BrokerManagedAsyncExecutorProvider(_brokerMetrics, 1, 1, 1);
+ ExecutorService executorService = provider.getExecutorService();
+
+ // verify that the executor has the expected properties
+ assertNotNull(executorService);
+ assertTrue(executorService instanceof ThreadPoolExecutor);
+
+ ThreadPoolExecutor threadPoolExecutor = (ThreadPoolExecutor) executorService;
+
+ assertEquals(1, threadPoolExecutor.getCorePoolSize());
+ assertEquals(1, threadPoolExecutor.getMaximumPoolSize());
+
+ BlockingQueue<Runnable> blockingQueue = threadPoolExecutor.getQueue();
+ assertNotNull(blockingQueue);
+ assertTrue(blockingQueue instanceof ArrayBlockingQueue);
+ assertEquals(0, blockingQueue.size());
+ assertEquals(1, blockingQueue.remainingCapacity());
+
+ RejectedExecutionHandler rejectedExecutionHandler = threadPoolExecutor.getRejectedExecutionHandler();
+ assertNotNull(rejectedExecutionHandler);
+ assertTrue(rejectedExecutionHandler
+ instanceof BrokerManagedAsyncExecutorProvider.BrokerThreadPoolRejectExecutionHandler);
+
+ // test that the executor actually executes tasks
+ AtomicInteger counter = new AtomicInteger();
+ for (int i = 0; i < 1; i++) {
+ threadPoolExecutor.execute(() -> counter.incrementAndGet());
+ }
+ assertEquals(counter.get(), 0);
Review Comment:
Thanks, this is fixed.
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167234729
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,20 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_THREAD_POOL = "pinot.broker.enable.bounded.threadPool";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_THREAD_POOL = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_MAX_POOL_SIZE = "pinot.broker.max.pool.size";
Review Comment:
Thanks Ankit, addressed in the second commit
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167298260
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -148,6 +156,15 @@ public void processSqlQueryPost(String query, @Suspended AsyncResponse asyncResp
BrokerResponse brokerResponse =
executeSqlQuery((ObjectNode) requestJson, makeHttpIdentity(requestContext), false);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (ServiceUnavailableException e) {
Review Comment:
Thanks Ankit, I just checked again. The ServiceUnavailableException is not getting propagated to the API and it is caught by the Grizzly HTTP server and handled internally. Users will receive a error code 503 (Service Unavailable).
I will fix it and propagate the exception to the API.
--
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] ankitsultana commented on pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #10614:
URL: https://github.com/apache/pinot/pull/10614#issuecomment-1514066204
@Jackie-Jiang : Yeah I had added those. Those configs control the thread-pool for the sync APIs afaiu. We mainly use them for tuning the thread-pool size for the pinot-controllers.
I think we don't really need to support that config for brokers. We could consider removing support for it to reduce confusion.
--
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] codecov-commenter commented on pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10614:
URL: https://github.com/apache/pinot/pull/10614#issuecomment-1508096820
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10614](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aa2f2f9) into [master](https://codecov.io/gh/apache/pinot/commit/13a792f03f267bf53e36fc06340d43d8fc1d7af4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (13a792f) will **decrease** coverage by `44.65%`.
> The diff coverage is `8.57%`.
```diff
@@ Coverage Diff @@
## master #10614 +/- ##
=============================================
- Coverage 69.03% 24.38% -44.65%
+ Complexity 6499 49 -6450
=============================================
Files 2106 2091 -15
Lines 113010 112613 -397
Branches 17027 16973 -54
=============================================
- Hits 78015 27462 -50553
- Misses 29515 82263 +52748
+ Partials 5480 2888 -2592
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.38% <8.57%> (?)` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `39.50% <0.00%> (-0.50%)` | :arrow_down: |
| [...ker/broker/BrokerManagedAsyncExecutorProvider.java](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jyb2tlck1hbmFnZWRBc3luY0V4ZWN1dG9yUHJvdmlkZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <ø> (-24.40%)` | :arrow_down: |
| [...pinot/broker/broker/BrokerAdminApiApplication.java](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jyb2tlckFkbWluQXBpQXBwbGljYXRpb24uamF2YQ==) | `81.57% <28.57%> (-5.38%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/BrokerMeter.java](https://codecov.io/gh/apache/pinot/pull/10614?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJNZXRlci5qYXZh) | `100.00% <100.00%> (ø)` | |
... and [1632 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10614/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1169419856
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
Review Comment:
Thanks Jackie, addressed in the third commit.
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167228845
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -148,6 +156,15 @@ public void processSqlQueryPost(String query, @Suspended AsyncResponse asyncResp
BrokerResponse brokerResponse =
executeSqlQuery((ObjectNode) requestJson, makeHttpIdentity(requestContext), false);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (ServiceUnavailableException e) {
Review Comment:
Hi Ankit, yes, this is returning to the user. The RejectExecutionHandler in BrokerManagedAsyncExecutorProvider will throw a ServiceUnavailableException exception and can be caught here.
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167333477
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -148,6 +156,15 @@ public void processSqlQueryPost(String query, @Suspended AsyncResponse asyncResp
BrokerResponse brokerResponse =
executeSqlQuery((ObjectNode) requestJson, makeHttpIdentity(requestContext), false);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (ServiceUnavailableException e) {
Review Comment:
Addressed.
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1169438135
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
+ "pinot.broker.enable.bounded.http.async.executor";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ "pinot.broker.http.async.executor.max.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ "pinot.broker.http.async.executor.core.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
+ "pinot.broker.http.async.executor.queue.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
Review Comment:
Hi Jasper, I didn't test the default values so they might be small. Can you share what might be a better number here?
--
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] jasperjiaguo commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "jasperjiaguo (via GitHub)" <gi...@apache.org>.
jasperjiaguo commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1169332961
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
+ "pinot.broker.enable.bounded.http.async.executor";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ "pinot.broker.http.async.executor.max.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ "pinot.broker.http.async.executor.core.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
+ "pinot.broker.http.async.executor.queue.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
Review Comment:
Would the default queue size of availableProcessors() * 2 being too small? During spiky request we may drop requests prematurely?
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1169420042
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -32,6 +32,8 @@ public enum BrokerMeter implements AbstractMetrics.Meter {
QUERIES("queries", false),
// These metrics track the exceptions caught during query execution in broker side.
+ // Query rejected by Jersey thread pool
+ QUERY_REJECTION_EXCEPTIONS("exceptions", true),
Review Comment:
Thanks, this is addressed.
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167636918
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerManagedAsyncExecutorProvider.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import javax.ws.rs.ServiceUnavailableException;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.common.metrics.BrokerMeter;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.glassfish.jersey.server.ManagedAsyncExecutor;
+import org.glassfish.jersey.spi.ThreadPoolExecutorProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * BrokerManagedAsyncExecutorProvider provides a bounded thread pool.
+ */
+@ManagedAsyncExecutor
+public class BrokerManagedAsyncExecutorProvider extends ThreadPoolExecutorProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(BrokerManagedAsyncExecutorProvider.class);
+
+ private static final String NAME = "broker-managed-async-executor";
+
+ private final BrokerMetrics _brokerMetrics;
+
+ private final int _maximumPoolSize;
+ private final int _corePoolSize;
+ private final int _queueSize;
+
+ public BrokerManagedAsyncExecutorProvider(
+ BrokerMetrics brokerMetrics,
+ int maximumPoolSize,
Review Comment:
addressed in the second commit
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167228845
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -148,6 +156,15 @@ public void processSqlQueryPost(String query, @Suspended AsyncResponse asyncResp
BrokerResponse brokerResponse =
executeSqlQuery((ObjectNode) requestJson, makeHttpIdentity(requestContext), false);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (ServiceUnavailableException e) {
Review Comment:
Hi Ankit, yes, this is returning to the user. The RejectExecutionHandler in BrokerManagedAsyncExecutorProvider will throw a ServiceUnavailableException exception and can be caught here.
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167333477
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -148,6 +156,15 @@ public void processSqlQueryPost(String query, @Suspended AsyncResponse asyncResp
BrokerResponse brokerResponse =
executeSqlQuery((ObjectNode) requestJson, makeHttpIdentity(requestContext), false);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (ServiceUnavailableException e) {
Review Comment:
Addressed. Please check the Test section
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -148,6 +156,15 @@ public void processSqlQueryPost(String query, @Suspended AsyncResponse asyncResp
BrokerResponse brokerResponse =
executeSqlQuery((ObjectNode) requestJson, makeHttpIdentity(requestContext), false);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (ServiceUnavailableException e) {
Review Comment:
Addressed. Please check the Test section in the summary
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1170448619
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerManagedAsyncExecutorProvider.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import javax.ws.rs.ServiceUnavailableException;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.common.metrics.BrokerMeter;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.glassfish.jersey.server.ManagedAsyncExecutor;
+import org.glassfish.jersey.spi.ThreadPoolExecutorProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * BrokerManagedAsyncExecutorProvider provides a bounded thread pool.
+ */
+@ManagedAsyncExecutor
+public class BrokerManagedAsyncExecutorProvider extends ThreadPoolExecutorProvider {
Review Comment:
addressed.
--
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] ankitsultana commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167223058
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,20 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_THREAD_POOL = "pinot.broker.enable.bounded.threadPool";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_THREAD_POOL = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_MAX_POOL_SIZE = "pinot.broker.max.pool.size";
Review Comment:
For defaults I think you can use Runtime.availableProcessors * 2, since we use that value as the default everywhere for thread counts.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -148,6 +156,15 @@ public void processSqlQueryPost(String query, @Suspended AsyncResponse asyncResp
BrokerResponse brokerResponse =
executeSqlQuery((ObjectNode) requestJson, makeHttpIdentity(requestContext), false);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (ServiceUnavailableException e) {
Review Comment:
Can you confirm the error message returned to the user?
I think you had mentioned that Grizzly was returning the error above the handler code so the error message being returned was "Internal error".
--
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] ankitsultana commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167400228
##########
pinot-broker/src/test/java/org/apache/pinot/broker/broker/BrokerManagedAsyncExecutorProviderTest.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+import java.util.Collections;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.ws.rs.ServiceUnavailableException;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.spi.metrics.PinotMetricUtils;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+
+public class BrokerManagedAsyncExecutorProviderTest {
+
+ public static BrokerMetrics _brokerMetrics;
+
+ @BeforeClass
+ public void setUp() {
+ _brokerMetrics = new BrokerMetrics(
+ CommonConstants.Broker.DEFAULT_METRICS_NAME_PREFIX,
+ PinotMetricUtils.getPinotMetricsRegistry(null),
+ CommonConstants.Broker.DEFAULT_ENABLE_TABLE_LEVEL_METRICS,
+ Collections.emptyList());
+ }
+
+ @Test
+ public void testExecutorService() throws InterruptedException, ExecutionException {
+ // create a new instance of the executor provider
+ BrokerManagedAsyncExecutorProvider provider = new BrokerManagedAsyncExecutorProvider(_brokerMetrics, 2, 2, 2);
+
+ // get the executor service
+ ThreadPoolExecutor executor = (ThreadPoolExecutor) provider.getExecutorService();
+
+ // submit a task to the executor service and wait for it to complete
+ Future<Integer> futureResult = executor.submit(() -> 1 + 1);
+ Integer result = futureResult.get();
+
+ // verify that the task was executed and returned the expected result
+ assertNotNull(result);
+ assertEquals((int) result, 2);
+
+ // wait for the executor service to shutdown
+ executor.shutdown();
+ executor.awaitTermination(1, TimeUnit.SECONDS);
+ }
+
+ @Test
+ public void testGet() {
+ BrokerManagedAsyncExecutorProvider provider = new BrokerManagedAsyncExecutorProvider(_brokerMetrics, 1, 1, 1);
+ ExecutorService executorService = provider.getExecutorService();
+
+ // verify that the executor has the expected properties
+ assertNotNull(executorService);
+ assertTrue(executorService instanceof ThreadPoolExecutor);
+
+ ThreadPoolExecutor threadPoolExecutor = (ThreadPoolExecutor) executorService;
+
+ assertEquals(1, threadPoolExecutor.getCorePoolSize());
+ assertEquals(1, threadPoolExecutor.getMaximumPoolSize());
+
+ BlockingQueue<Runnable> blockingQueue = threadPoolExecutor.getQueue();
+ assertNotNull(blockingQueue);
+ assertTrue(blockingQueue instanceof ArrayBlockingQueue);
+ assertEquals(0, blockingQueue.size());
+ assertEquals(1, blockingQueue.remainingCapacity());
+
+ RejectedExecutionHandler rejectedExecutionHandler = threadPoolExecutor.getRejectedExecutionHandler();
+ assertNotNull(rejectedExecutionHandler);
+ assertTrue(rejectedExecutionHandler
+ instanceof BrokerManagedAsyncExecutorProvider.BrokerThreadPoolRejectExecutionHandler);
+
+ // test that the executor actually executes tasks
+ AtomicInteger counter = new AtomicInteger();
+ for (int i = 0; i < 1; i++) {
+ threadPoolExecutor.execute(() -> counter.incrementAndGet());
+ }
+ assertEquals(counter.get(), 0);
Review Comment:
I think you need to do `assertEquals(counter.get(), 1)`.
Either ways, there's a race condition here which can introduce flakiness in tests. You can use a `CountDownLatch` instead.
##########
pinot-broker/src/test/java/org/apache/pinot/broker/broker/BrokerManagedAsyncExecutorProviderTest.java:
##########
@@ -0,0 +1,120 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+import java.util.Collections;
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.ws.rs.ServiceUnavailableException;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.spi.metrics.PinotMetricUtils;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+
+public class BrokerManagedAsyncExecutorProviderTest {
+
+ public static BrokerMetrics _brokerMetrics;
+
+ @BeforeClass
+ public void setUp() {
+ _brokerMetrics = new BrokerMetrics(
+ CommonConstants.Broker.DEFAULT_METRICS_NAME_PREFIX,
+ PinotMetricUtils.getPinotMetricsRegistry(null),
+ CommonConstants.Broker.DEFAULT_ENABLE_TABLE_LEVEL_METRICS,
+ Collections.emptyList());
+ }
+
+ @Test
+ public void testExecutorService() throws InterruptedException, ExecutionException {
+ // create a new instance of the executor provider
+ BrokerManagedAsyncExecutorProvider provider = new BrokerManagedAsyncExecutorProvider(_brokerMetrics, 2, 2, 2);
+
+ // get the executor service
+ ThreadPoolExecutor executor = (ThreadPoolExecutor) provider.getExecutorService();
+
+ // submit a task to the executor service and wait for it to complete
+ Future<Integer> futureResult = executor.submit(() -> 1 + 1);
+ Integer result = futureResult.get();
+
+ // verify that the task was executed and returned the expected result
+ assertNotNull(result);
+ assertEquals((int) result, 2);
+
+ // wait for the executor service to shutdown
+ executor.shutdown();
+ executor.awaitTermination(1, TimeUnit.SECONDS);
+ }
+
+ @Test
+ public void testGet() {
+ BrokerManagedAsyncExecutorProvider provider = new BrokerManagedAsyncExecutorProvider(_brokerMetrics, 1, 1, 1);
+ ExecutorService executorService = provider.getExecutorService();
+
+ // verify that the executor has the expected properties
+ assertNotNull(executorService);
+ assertTrue(executorService instanceof ThreadPoolExecutor);
+
+ ThreadPoolExecutor threadPoolExecutor = (ThreadPoolExecutor) executorService;
+
+ assertEquals(1, threadPoolExecutor.getCorePoolSize());
+ assertEquals(1, threadPoolExecutor.getMaximumPoolSize());
+
+ BlockingQueue<Runnable> blockingQueue = threadPoolExecutor.getQueue();
+ assertNotNull(blockingQueue);
+ assertTrue(blockingQueue instanceof ArrayBlockingQueue);
+ assertEquals(0, blockingQueue.size());
+ assertEquals(1, blockingQueue.remainingCapacity());
+
+ RejectedExecutionHandler rejectedExecutionHandler = threadPoolExecutor.getRejectedExecutionHandler();
+ assertNotNull(rejectedExecutionHandler);
+ assertTrue(rejectedExecutionHandler
+ instanceof BrokerManagedAsyncExecutorProvider.BrokerThreadPoolRejectExecutionHandler);
+
+ // test that the executor actually executes tasks
+ AtomicInteger counter = new AtomicInteger();
+ for (int i = 0; i < 1; i++) {
+ threadPoolExecutor.execute(() -> counter.incrementAndGet());
+ }
+ assertEquals(counter.get(), 0);
+ }
+
+ @Test(expectedExceptions = ServiceUnavailableException.class)
+ public void testRejectHandler() {
+ BrokerManagedAsyncExecutorProvider provider = new BrokerManagedAsyncExecutorProvider(_brokerMetrics, 1, 1, 1);
+ ThreadPoolExecutor threadPoolExecutor = (ThreadPoolExecutor) provider.getExecutorService();
+
+ // test the rejection policy
+ AtomicInteger counter = new AtomicInteger();
+ for (int i = 0; i < 10; i++) {
+ threadPoolExecutor.execute(() -> counter.incrementAndGet());
Review Comment:
Same as above. CountDownLatch can be used to ensure the test works as expected.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java:
##########
@@ -105,6 +105,18 @@ protected void configure() {
bind(accessFactory).to(AccessControlFactory.class);
}
});
+ boolean enableBoundedThreadPool = brokerConf
+ .getProperty(CommonConstants.Broker.CONFIG_OF_ENABLE_BOUNDED_THREAD_POOL,
+ CommonConstants.Broker.DEFAULT_ENABLE_BOUNDED_THREAD_POOL);
+ if (enableBoundedThreadPool) {
Review Comment:
nit: move initialization to a private method
```
register(buildBrokerManagedAsyncExecutorProvider(...))
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerManagedAsyncExecutorProvider.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.broker;
+
+
+import java.util.concurrent.ArrayBlockingQueue;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.RejectedExecutionHandler;
+import java.util.concurrent.ThreadPoolExecutor;
+import javax.ws.rs.ServiceUnavailableException;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.common.metrics.BrokerMeter;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.glassfish.jersey.server.ManagedAsyncExecutor;
+import org.glassfish.jersey.spi.ThreadPoolExecutorProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * BrokerManagedAsyncExecutorProvider provides a bounded thread pool.
+ */
+@ManagedAsyncExecutor
+public class BrokerManagedAsyncExecutorProvider extends ThreadPoolExecutorProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(BrokerManagedAsyncExecutorProvider.class);
+
+ private static final String NAME = "broker-managed-async-executor";
+
+ private final BrokerMetrics _brokerMetrics;
+
+ private final int _maximumPoolSize;
+ private final int _corePoolSize;
+ private final int _queueSize;
+
+ public BrokerManagedAsyncExecutorProvider(
+ BrokerMetrics brokerMetrics,
+ int maximumPoolSize,
Review Comment:
nit: change order to `corePoolSize, maximumPoolSize, queueSize, brokerMetrics` since ThreadPoolExecutor takes corePoolSize first followed by max pool size.
--
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] Jackie-Jiang commented on pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10614:
URL: https://github.com/apache/pinot/pull/10614#issuecomment-1514023056
@MeihanLi By reading the pinot doc, I just realized that we already have a set of configs (`pinot.broker.http.server.thread.pool.corePoolSize` and `pinot.broker.http.server.thread.pool.maxPoolSize`) for http servers, which can be applied to all components. Are they for the same purpose? If so, are we able to integrate the new config at the same place? Currently they are applied in 2 different places, which is quite hard to manage.
cc @apucher who has more context on how to config the rest server
--
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] ankitsultana commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1168084649
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,20 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_THREAD_POOL = "pinot.broker.enable.bounded.threadPool";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_THREAD_POOL = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_MAX_POOL_SIZE = "pinot.broker.max.pool.size";
Review Comment:
Config name can be changed to: `pinot.broker.async.executor.max.pool.size` (similarly for other configs).
Since broker uses a few other thread-pools it's best to be explicit.
--
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] MeihanLi closed pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi closed pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
URL: https://github.com/apache/pinot/pull/10614
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1167636870
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BrokerAdminApiApplication.java:
##########
@@ -105,6 +105,18 @@ protected void configure() {
bind(accessFactory).to(AccessControlFactory.class);
}
});
+ boolean enableBoundedThreadPool = brokerConf
+ .getProperty(CommonConstants.Broker.CONFIG_OF_ENABLE_BOUNDED_THREAD_POOL,
+ CommonConstants.Broker.DEFAULT_ENABLE_BOUNDED_THREAD_POOL);
+ if (enableBoundedThreadPool) {
Review Comment:
Thanks, addressed in the second commit.
--
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] MeihanLi commented on pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #10614:
URL: https://github.com/apache/pinot/pull/10614#issuecomment-1513820765
> LGTM otherwise
Thanks @Jackie-Jiang , comments are all addressed. Can you help take another look. Thanks
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1170534458
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
+ "pinot.broker.enable.bounded.http.async.executor";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ "pinot.broker.http.async.executor.max.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ "pinot.broker.http.async.executor.core.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
+ "pinot.broker.http.async.executor.queue.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
Review Comment:
@jasperjiaguo I set the default value to Integer.MAX_VALUE, let me know if this looks good to you. The change is in the last commit.
--
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] MeihanLi commented on pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #10614:
URL: https://github.com/apache/pinot/pull/10614#issuecomment-1513942681
> @MeihanLi Can you help update the pinot documentation to include this new feature?
Thanks Jackie for merging it, I will update the documentation soon
--
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] Jackie-Jiang commented on pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10614:
URL: https://github.com/apache/pinot/pull/10614#issuecomment-1513925019
@MeihanLi Can you help updating the pinot documentation to include this new feature?
--
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] MeihanLi commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1168171520
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,20 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_THREAD_POOL = "pinot.broker.enable.bounded.threadPool";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_THREAD_POOL = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_MAX_POOL_SIZE = "pinot.broker.max.pool.size";
Review Comment:
Thanks Ankit, addressed.
--
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] wirybeaver commented on a diff in pull request #10614: [New Feature] Add new configuration options which allows broker to use a bounded Jersey ThreadPool
Posted by "wirybeaver (via GitHub)" <gi...@apache.org>.
wirybeaver commented on code in PR #10614:
URL: https://github.com/apache/pinot/pull/10614#discussion_r1169114326
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -243,6 +243,26 @@ public static class Broker {
Math.max(1, Math.min(10, Runtime.getRuntime().availableProcessors() / 2));
// Same logic as CombineOperatorUtils
+ // Config for Jersey ThreadPoolExecutorProvider.
+ // By default, Jersey uses the default unbounded thread pool to process queries.
+ // By enabling it, BrokerManagedAsyncExecutorProvider will be used to create a bounded thread pool.
+ public static final String CONFIG_OF_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR =
+ "pinot.broker.enable.bounded.http.async.executor";
+ public static final boolean DEFAULT_ENABLE_BOUNDED_HTTP_ASYNC_EXECUTOR = false;
+ // Default capacities for the bounded thread pool
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ "pinot.broker.http.async.executor.max.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_MAX_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ "pinot.broker.http.async.executor.core.pool.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_CORE_POOL_SIZE =
+ Runtime.getRuntime().availableProcessors() * 2;
+ public static final String CONFIG_OF_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
+ "pinot.broker.http.async.executor.queue.size";
+ public static final int DEFAULT_HTTP_ASYNC_EXECUTOR_QUEUE_SIZE =
Review Comment:
queue size actually is bounded by memory. but I think availableProcessors() * 2 is OK.
--
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