executor-spi #13921
executor-spi #13921
Conversation
| LOGGER.warn("Duplicate provider for id '{}': {} and {}", plugin.id(), old, provider); | ||
| } | ||
| } | ||
| if (PROVIDERS.isEmpty()) { |
There was a problem hiding this comment.
Isn't it guaranteed that fixed and cached will be available ?
There was a problem hiding this comment.
Yes, unless there is a bug, in which case here we will have a better error at startup time instead of a null pointer exception.
| if (PROVIDERS.isEmpty()) { | ||
| throw new IllegalStateException("No executor service providers found"); | ||
| } else if (PROVIDERS.containsKey("cached")) { | ||
| DEFAULT_PROVIDER = "cached"; |
There was a problem hiding this comment.
An alternative is to have a constant called DEFAULT_QUERY_EXECUTOR_OPCHAIN_EXECUTOR. Then in QueryRunner, get thhe executor as config.get(CONFIG_OF_QUERY_EXECUTOR_OPCHAIN_EXECUTOR, DEFAULT_QUERY_EXECUTOR_OPCHAIN_EXECUTOR. I find this simpler. Are there pros to this approach over a default constant ?
There was a problem hiding this comment.
Having to move the default to QueryRunner is worse because in the future we expect to have more places using this API, which would end up having to provide the default everywhere.
Anyway that can be done right now. We provide two create methods, one that includes a default type and one that does not. The later use the default calculated here.
Is this default calculated in a unnecessary complex way? Totally. That was my bad. I've just picked the code from #13619 and simplify it a bit instead of starting from scratch. Why the code in #13619 is more complex? Because there I wanted to use virtual threads by default when using Java 21.
I'm going to simplify the code because the idea of using virtual threads by default may not be the best idea ever given we are not sure if we are actually pinning the carrier somewhere or not. The ideal solution in the future would be to have an init() method in this class so we can decide which executor use by default depending on the configuration we receive at startup time.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13921 +/- ##
============================================
- Coverage 61.75% 57.98% -3.78%
+ Complexity 207 197 -10
============================================
Files 2436 2589 +153
Lines 133233 142539 +9306
Branches 20636 21892 +1256
============================================
+ Hits 82274 82646 +372
- Misses 44911 53426 +8515
- Partials 6048 6467 +419
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Jackie-Jiang
added
feature
documentation
Configuration
This PR adds a new SPI that can be extended to support new executor services. By default 2 executors are provided: cached, the default, which is a wrapper on top of Executors.newCachedThreadPool. fixed, which is a wrapper on top of Executors.newFixedThreadPool. Right now only org.apache.pinot.query.runtime.QueryRunner uses this SPI.
This reverts commit 977773c3883ca5e24f8df82d4da51a677fd69012.
This PR adds a new SPI that can be extended to support new executor services.
By default 2 executors are provided:
cached, the default, which is a wrapper on top ofExecutors.newCachedThreadPool.fixed, which is a wrapper on top ofExecutors.newFixedThreadPool.Right now only
org.apache.pinot.query.runtime.QueryRunneruses this SPI.This PR is a simplification of #13619. The code is almost the same, but does not provide an implementation for virtual threads in order to do not fall into the Java 21 compilation issues. The plan is to add this plugin once we solve that problem. Meanwhile virtual threads can be tested by creating a new (very simple) plugin externally.