You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "priyen-stripe (via GitHub)" <gi...@apache.org> on 2025/11/12 19:41:53 UTC
[PR] add gauges for mse thread usage and limit when the limiting feature is used [pinot]
priyen-stripe opened a new pull request, #17198:
URL: https://github.com/apache/pinot/pull/17198
Currently, when you use the multi stage engine thread limiting feature, (via configs such as below)
```
pinot.server.query.executor.mse.max.execution.threads=1000
pinot.server.query.executor.mse.max.execution.threads.exceed.strategy=ERROR
```
you have no insights, through metrics, what's the current usage or how close you are to the limits. So without this, we are also unable to catch issues inadvance of when users might see them
This PR adds two gauges:
MSE_THREAD_USAGE_MAX("threads", true, "Maximum allowed threads for multi-stage executor"),
MSE_THREAD_USAGE_CURRENT("threads", true, "Current number of threads in use by multi-stage executor");
Also updates the test using similiar code to existing tests
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2543798601
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -38,12 +39,21 @@ public class HardLimitExecutor extends DecoratorExecutorService {
private final AtomicInteger _running;
private final int _max;
private final QueryThreadExceedStrategy _exceedStrategy;
+ private final Consumer<Integer> _currentUsageGaugeUpdater;
- public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy,
+ Consumer<Integer> maxUsageGaugeUpdater, Consumer<Integer> currentUsageGaugeUpdater) {
super(executorService);
_running = new AtomicInteger(0);
_max = max;
_exceedStrategy = exceedStrategy;
+ _currentUsageGaugeUpdater = currentUsageGaugeUpdater;
+ maxUsageGaugeUpdater.accept(max);
+ _currentUsageGaugeUpdater.accept(0);
+ }
+
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ this(max, executorService, exceedStrategy, max1 -> { }, current -> { });
Review Comment:
It is not violating check style, but just not follow Pinot Style (follow [this](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#set-up-ide) to set it up in your IDE)
--
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
Re: [PR] add gauges for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2519572598
##########
pinot-spi/src/test/java/org/apache/pinot/spi/executor/HardLimitExecutorTest.java:
##########
@@ -127,4 +130,48 @@ public void testGetMultiStageExecutorHardLimit() {
PinotConfiguration config5 = new PinotConfiguration(configMap5);
assertEquals(HardLimitExecutor.getMultiStageExecutorHardLimit(config5), -1);
}
+
+ @Test
+ public void testGaugeTracking()
+ throws Exception {
+ AtomicInteger maxGauge = new AtomicInteger(-1);
+ AtomicInteger currentGauge = new AtomicInteger(-1);
+
+ HardLimitExecutor ex = new HardLimitExecutor(2, Executors.newCachedThreadPool(),
+ QueryThreadExceedStrategy.ERROR,
+ max -> maxGauge.set(max),
+ current -> currentGauge.set(current));
+
+ CyclicBarrier barrier = new CyclicBarrier(3);
+
+ try {
+ assertEquals(maxGauge.get(), 2);
+ assertEquals(currentGauge.get(), 0);
+
+ ex.execute(() -> {
+ try {
+ barrier.await();
+ Thread.sleep(Long.MAX_VALUE);
Review Comment:
the sleep/do nothing just follows the other examples in the test that already exist
shutdownNow in the finally cleans it up
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2543794369
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -38,18 +38,33 @@ public class HardLimitExecutor extends DecoratorExecutorService {
private final AtomicInteger _running;
private final int _max;
private final QueryThreadExceedStrategy _exceedStrategy;
+ private final Runnable _taskRejectionCounter;
- public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy,
Review Comment:
Not introduced in this PR, but this class doesn't really belong to `pinot-spi`. I think you can move this class into `pinot-query-runtime` and directly update the server metric within the class
--
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
Re: [PR] add gauges for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2519794373
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -111,11 +123,13 @@ protected Runnable decorate(Runnable task) {
checkTaskAllowed();
return () -> {
checkTaskAllowed();
- _running.getAndIncrement();
+ int currentCount = _running.getAndIncrement();
+ _currentUsageGaugeUpdater.accept(currentCount + 1);
Review Comment:
can you explain more? then what will be doing the metric emitting?
--
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
Re: [PR] add gauges for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17198:
URL: https://github.com/apache/pinot/pull/17198#issuecomment-3523773593
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17198?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `55.55556%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.92%. Comparing base ([`c6d4f1c`](https://app.codecov.io/gh/apache/pinot/commit/c6d4f1c5aab5bf64c4b351f479d9ae95e3483f81?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a53566d`](https://app.codecov.io/gh/apache/pinot/commit/a53566d11872df16841461d3642e4644779316ef?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17198?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...g/apache/pinot/spi/executor/HardLimitExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/17198?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fexecutor%2FHardLimitExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZXhlY3V0b3IvSGFyZExpbWl0RXhlY3V0b3IuamF2YQ==) | 61.53% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...va/org/apache/pinot/query/runtime/QueryRunner.java](https://app.codecov.io/gh/apache/pinot/pull/17198?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2FQueryRunner.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9RdWVyeVJ1bm5lci5qYXZh) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (c6d4f1c) and HEAD (a53566d). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (c6d4f1c) | HEAD (a53566d) |
>|------|------|------|
>|java-21|4|1|
>|unittests|2|1|
>|temurin|7|1|
>|unittests2|1|0|
>|java-11|3|0|
>|integration|5|0|
>|integration2|1|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17198 +/- ##
============================================
- Coverage 63.14% 55.92% -7.22%
+ Complexity 1427 701 -726
============================================
Files 3121 2434 -687
Lines 184760 136498 -48262
Branches 28327 21809 -6518
============================================
- Hits 116664 76336 -40328
+ Misses 59069 53757 -5312
+ Partials 9027 6405 -2622
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <55.55%> (-0.02%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <55.55%> (-7.22%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <55.55%> (-7.22%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <55.55%> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17198?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2519957614
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -111,11 +123,13 @@ protected Runnable decorate(Runnable task) {
checkTaskAllowed();
return () -> {
checkTaskAllowed();
- _running.getAndIncrement();
+ int currentCount = _running.getAndIncrement();
+ _currentUsageGaugeUpdater.accept(currentCount + 1);
Review Comment:
actually, I think I know what you mean - pushed a commit for it
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #17198:
URL: https://github.com/apache/pinot/pull/17198#issuecomment-3524037235
@jackie the sleep is needed, without Thread.sleep, the tasks complete immediately, so currentGauge drops to 0 before we can check it. I think for similar reasons is why the existing tests use it too
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2551056981
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -38,18 +38,33 @@ public class HardLimitExecutor extends DecoratorExecutorService {
private final AtomicInteger _running;
private final int _max;
private final QueryThreadExceedStrategy _exceedStrategy;
+ private final Runnable _taskRejectionCounter;
- public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy,
Review Comment:
do you mean this class as in (HardLimitExecutor?)
--
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
Re: [PR] add gauges for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2519762268
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -38,12 +39,21 @@ public class HardLimitExecutor extends DecoratorExecutorService {
private final AtomicInteger _running;
private final int _max;
private final QueryThreadExceedStrategy _exceedStrategy;
+ private final Consumer<Integer> _currentUsageGaugeUpdater;
- public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy,
+ Consumer<Integer> maxUsageGaugeUpdater, Consumer<Integer> currentUsageGaugeUpdater) {
super(executorService);
_running = new AtomicInteger(0);
_max = max;
_exceedStrategy = exceedStrategy;
+ _currentUsageGaugeUpdater = currentUsageGaugeUpdater;
+ maxUsageGaugeUpdater.accept(max);
+ _currentUsageGaugeUpdater.accept(0);
+ }
+
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ this(max, executorService, exceedStrategy, max1 -> { }, current -> { });
Review Comment:
(format) Seems this is not following pinot style. Same for other places
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerGauge.java:
##########
@@ -153,7 +153,11 @@ public enum ServerGauge implements AbstractMetrics.Gauge {
// ThrottleOnCriticalHeapUsageExecutor metrics
THROTTLE_EXECUTOR_QUEUE_SIZE("count", true,
- "Current number of tasks in the throttle executor queue");
+ "Current number of tasks in the throttle executor queue"),
+
+ // Multi-stage executor thread usage metrics
+ MSE_THREAD_USAGE_MAX("threads", true, "Maximum allowed threads for multi-stage executor"),
Review Comment:
Since this is fixed, I feel it is not worth adding given the overhead of processing gauge. We can log it when HardLimitExecutor is initialized
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -111,11 +123,13 @@ protected Runnable decorate(Runnable task) {
checkTaskAllowed();
return () -> {
checkTaskAllowed();
- _running.getAndIncrement();
+ int currentCount = _running.getAndIncrement();
+ _currentUsageGaugeUpdater.accept(currentCount + 1);
Review Comment:
Ideally this should be a callback gauge. You may add a method in this class to return the current thread usage
##########
pinot-spi/src/test/java/org/apache/pinot/spi/executor/HardLimitExecutorTest.java:
##########
@@ -127,4 +130,48 @@ public void testGetMultiStageExecutorHardLimit() {
PinotConfiguration config5 = new PinotConfiguration(configMap5);
assertEquals(HardLimitExecutor.getMultiStageExecutorHardLimit(config5), -1);
}
+
+ @Test
+ public void testGaugeTracking()
+ throws Exception {
+ AtomicInteger maxGauge = new AtomicInteger(-1);
+ AtomicInteger currentGauge = new AtomicInteger(-1);
+
+ HardLimitExecutor ex = new HardLimitExecutor(2, Executors.newCachedThreadPool(),
+ QueryThreadExceedStrategy.ERROR,
+ max -> maxGauge.set(max),
+ current -> currentGauge.set(current));
+
+ CyclicBarrier barrier = new CyclicBarrier(3);
+
+ try {
+ assertEquals(maxGauge.get(), 2);
+ assertEquals(currentGauge.get(), 0);
+
+ ex.execute(() -> {
+ try {
+ barrier.await();
+ Thread.sleep(Long.MAX_VALUE);
Review Comment:
If this is not required, let's remove it. I believe we want to test that barrier is not reached, and sleep prevents the purpose
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2519862816
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -38,12 +39,21 @@ public class HardLimitExecutor extends DecoratorExecutorService {
private final AtomicInteger _running;
private final int _max;
private final QueryThreadExceedStrategy _exceedStrategy;
+ private final Consumer<Integer> _currentUsageGaugeUpdater;
- public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy,
+ Consumer<Integer> maxUsageGaugeUpdater, Consumer<Integer> currentUsageGaugeUpdater) {
super(executorService);
_running = new AtomicInteger(0);
_max = max;
_exceedStrategy = exceedStrategy;
+ _currentUsageGaugeUpdater = currentUsageGaugeUpdater;
+ maxUsageGaugeUpdater.accept(max);
+ _currentUsageGaugeUpdater.accept(0);
+ }
+
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ this(max, executorService, exceedStrategy, max1 -> { }, current -> { });
Review Comment:
hmm I am running checkstyle it is getting 0 validations
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2551577088
##########
pinot-spi/src/main/java/org/apache/pinot/spi/executor/HardLimitExecutor.java:
##########
@@ -38,18 +38,33 @@ public class HardLimitExecutor extends DecoratorExecutorService {
private final AtomicInteger _running;
private final int _max;
private final QueryThreadExceedStrategy _exceedStrategy;
+ private final Runnable _taskRejectionCounter;
- public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy) {
+ public HardLimitExecutor(int max, ExecutorService executorService, QueryThreadExceedStrategy exceedStrategy,
Review Comment:
Yes. `HardLimitExecutor` doesn't belong to `pinot-spi`
--
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
Re: [PR] add gauges for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on code in PR #17198:
URL: https://github.com/apache/pinot/pull/17198#discussion_r2519791802
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerGauge.java:
##########
@@ -153,7 +153,11 @@ public enum ServerGauge implements AbstractMetrics.Gauge {
// ThrottleOnCriticalHeapUsageExecutor metrics
THROTTLE_EXECUTOR_QUEUE_SIZE("count", true,
- "Current number of tasks in the throttle executor queue");
+ "Current number of tasks in the throttle executor queue"),
+
+ // Multi-stage executor thread usage metrics
+ MSE_THREAD_USAGE_MAX("threads", true, "Maximum allowed threads for multi-stage executor"),
Review Comment:
we would like to keep this as without it you can't compute the % of limit used. Ideally we can setup alerts/detectors when users are approaching their limits and action it before they notice. If this metric is not available, we wont be able to
do you antipciate processing overhead even if its just set once during constructor?
--
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
Re: [PR] add gauges + rejections counter for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #17198:
URL: https://github.com/apache/pinot/pull/17198#issuecomment-3524534907
test failures look unrelated, can you re-run @Jackie-Jiang ?
--
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
Re: [PR] add gauges for mse thread usage and limit when the limiting feature is used [pinot]
Posted by "priyen-stripe (via GitHub)" <gi...@apache.org>.
priyen-stripe commented on PR #17198:
URL: https://github.com/apache/pinot/pull/17198#issuecomment-3523688149
cc @yashmayya @Jackie-Jiang are you able to review this?
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