You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/04/10 18:53:51 UTC
[PR] Add OOM pause mechanism to pause query threads before killing on critical heap [pinot]
yashmayya opened a new pull request, #18163:
URL: https://github.com/apache/pinot/pull/18163
### Summary
- When heap transitions from below-critical to critical, pauses all query execution threads for a configurable timeout and calls `System.gc()` (only once, on first transition to critical), giving the JVM breathing room to reclaim memory before resorting to query kills.
- If heap recovers below critical during the pause window, threads resume with no query killed.
- If heap stays critical after timeout, the existing kill-most-expensive-query logic proceeds.
- Off by default, dynamically toggleable via cluster config (no restart required).
<hr>
### Overhead
- Happy path (no OOM pressure): `waitIfPaused()` is called at every sampling checkpoint and costs a single volatile read of `_pauseActive`. No lock, no contention.
- Triggering path (critical heap): Query threads block on a `ReentrantLock` + `Condition`. Lock contention is not a concern because:
- Threads acquire the lock only to enter `Condition.await()`, releasing it immediately - hold time is near-zero.
- `activatePause()` (watcher thread) is lock-free - it writes two volatiles with correct ordering.
- This only activates when the JVM is already under critical heap pressure and queries are about to be killed anyway.
<hr>
### Configuration
```
┌────────────────────────────────────────────────┬─────────┬─────────┬─────────────────────────────────────────────────────────┐
│ Config key │ Type │ Default │ Description │
├────────────────────────────────────────────────┼─────────┼─────────┼─────────────────────────────────────────────────────────┤
│ accounting.oom.critical.query.pause.enabled │ boolean │ false │ Enable pausing query threads before killing on critical │
│ │ │ │ heap │
├────────────────────────────────────────────────┼─────────┼─────────┼─────────────────────────────────────────────────────────┤
│ accounting.oom.critical.query.pause.timeout.ms │ long │ 1000 │ How long (ms) to pause before proceeding with kill │
└────────────────────────────────────────────────┴─────────┴─────────┴─────────────────────────────────────────────────────────┘
```
Both are dynamically updatable via Helix cluster config.
<hr>
### State transitions
- Below critical → Critical (enabled): Activate pause, call `System.gc()`
- Critical → Critical (within timeout): No-op, let GC work
- Critical → Critical (timeout expired): Clear pause, kill most expensive query
- Critical → Below critical (pause active): Clear pause, resume threads, no kill
- Any → Panic: Kill all queries first, then clear pause
- Below critical → Critical (disabled): Existing behavior (immediate kill of most expensive query)
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18163:
URL: https://github.com/apache/pinot/pull/18163#issuecomment-4226324388
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18163?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 `76.47059%` with `16 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.38%. Comparing base ([`e7ea035`](https://app.codecov.io/gh/apache/pinot/commit/e7ea035cc76874c9988fa7d377e46aeac5bb8d4c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bd14624`](https://app.codecov.io/gh/apache/pinot/commit/bd1462401090ad4ead42805b7ce34dffd7d03e99?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 21 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18163?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pinot/core/accounting/QueryResourceAggregator.java](https://app.codecov.io/gh/apache/pinot/pull/18163?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Faccounting%2FQueryResourceAggregator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL1F1ZXJ5UmVzb3VyY2VBZ2dyZWdhdG9yLmphdmE=) | 80.85% | [5 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18163?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ache/pinot/core/accounting/QueryMonitorConfig.java](https://app.codecov.io/gh/apache/pinot/pull/18163?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Faccounting%2FQueryMonitorConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL1F1ZXJ5TW9uaXRvckNvbmZpZy5qYXZh) | 75.00% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18163?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ore/accounting/ResourceUsageAccountantFactory.java](https://app.codecov.io/gh/apache/pinot/pull/18163?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Faccounting%2FResourceUsageAccountantFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL1Jlc291cmNlVXNhZ2VBY2NvdW50YW50RmFjdG9yeS5qYXZh) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18163?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 (e7ea035) and HEAD (bd14624). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (e7ea035) | HEAD (bd14624) |
>|------|------|------|
>|java-21|4|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|5|0|
>|integration1|2|0|
>|integration2|1|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18163 +/- ##
============================================
- Coverage 63.96% 55.38% -8.58%
+ Complexity 1617 826 -791
============================================
Files 3179 2518 -661
Lines 193741 144767 -48974
Branches 29926 23258 -6668
============================================
- Hits 123918 80183 -43735
+ Misses 60031 57679 -2352
+ Partials 9792 6905 -2887
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18163/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/18163/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/18163/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/18163/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/18163/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/18163/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/18163/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.38% <76.47%> (-8.55%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18163/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.38% <76.47%> (-8.58%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18163/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.38% <76.47%> (-8.57%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18163/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.38% <76.47%> (-0.47%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18163/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/18163?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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3081724642
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1752,6 +1746,12 @@ public static class Accounting {
public static final String CONFIG_OF_QUERY_KILLED_METRIC_ENABLED = "accounting.query.killed.metric.enabled";
public static final boolean DEFAULT_QUERY_KILLED_METRIC_ENABLED = false;
+ public static final String CONFIG_OF_OOM_PAUSE_ENABLED = "accounting.oom.critical.query.pause.enabled";
+ public static final boolean DEFAULT_OOM_PAUSE_ENABLED = false;
+
+ public static final String CONFIG_OF_OOM_PAUSE_TIMEOUT_MS = "accounting.oom.critical.query.pause.timeout.ms";
Review Comment:
Yeah I can merge the configs into one, but I'm not sure `query.kill.grace.period` is the right terminology? It's not exactly a "grace period" we're offering the threads - which sounds like we're waiting for them to complete execution gracefully - we're actually pausing execution instead.
As for the point on panic, IMO that would significantly muddy the panic semantics. I'd argue panic should stay as immediate kill-all with no pause since the JVM is almost out of memory and any delay could tip into OOM.
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3083188522
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1752,6 +1746,12 @@ public static class Accounting {
public static final String CONFIG_OF_QUERY_KILLED_METRIC_ENABLED = "accounting.query.killed.metric.enabled";
public static final boolean DEFAULT_QUERY_KILLED_METRIC_ENABLED = false;
+ public static final String CONFIG_OF_OOM_PAUSE_ENABLED = "accounting.oom.critical.query.pause.enabled";
+ public static final boolean DEFAULT_OOM_PAUSE_ENABLED = false;
+
+ public static final String CONFIG_OF_OOM_PAUSE_TIMEOUT_MS = "accounting.oom.critical.query.pause.timeout.ms";
Review Comment:
How about `accounting.oom.pre.query.kill.pause.duration.ms`? I feel `pause.timeout` is a little bit ambiguous.
I remember seeing memory usage directly jumping to panic sometimes. I agree it feels dangerous to not immediately kill on panic, but maybe it is good to at least have that option so we can try it out
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3094816416
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -75,6 +78,15 @@ enum TriggeringLevel {
private Map<String, QueryResourceTrackerImpl> _queryResourceUsages;
private TriggeringLevel _triggeringLevel = TriggeringLevel.Normal;
+ // Track previous triggering level to detect transitions into critical
+ private TriggeringLevel _previousTriggeringLevel = TriggeringLevel.Normal;
+
+ // OOM pause state - lock+condition for blocking query threads at critical heap level
+ private final ReentrantLock _pauseLock = new ReentrantLock();
+ private final Condition _pauseCondition = _pauseLock.newCondition();
+ private volatile boolean _pauseActive = false;
+ private volatile long _pauseDeadlineMs = 0;
Review Comment:
Yes good point, this is left over from the previous implementation where the other threads were also checking the deadline to unpause themselves.
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3095348288
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryThreadContext.java:
##########
@@ -259,6 +263,7 @@ public static void checkTermination(Supplier<String> scopeSupplier) {
// NOTE: In production code, threadContext should never be null. It might be null in tests when QueryThreadContext
// is not set up.
if (threadContext != null) {
+ threadContext.waitIfPausedInternal();
Review Comment:
Done
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3094800260
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -179,7 +211,22 @@ public void postAggregate() {
}
switch (_triggeringLevel) {
case HeapMemoryCritical:
- killMostExpensiveQuery();
+ QueryMonitorConfig config = _queryMonitorConfig.get();
+ if (_pauseActive) {
+ // Pause is active - check if grace period expired
+ if (System.currentTimeMillis() >= _pauseDeadlineMs) {
+ killMostExpensiveQuery();
+ clearPause();
+ }
+ // else: still in grace period, skip kill this cycle
+ } else if (config.isOomPreQueryKillPauseEnabled()
Review Comment:
I don't think this extreme edge case is worth introducing additional complexity for IMO, it's easier to reason about this way.
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3089997447
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryThreadContext.java:
##########
@@ -259,6 +263,7 @@ public static void checkTermination(Supplier<String> scopeSupplier) {
// NOTE: In production code, threadContext should never be null. It might be null in tests when QueryThreadContext
// is not set up.
if (threadContext != null) {
+ threadContext.waitIfPausedInternal();
Review Comment:
Discussed offline. Let's still check termination first, then when entering the pause, check termination again before exiting the pause. Pause should be done after the sampling
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1759,6 +1759,14 @@ public static class Accounting {
public static final String CONFIG_OF_QUERY_KILLED_METRIC_ENABLED = "accounting.query.killed.metric.enabled";
public static final boolean DEFAULT_QUERY_KILLED_METRIC_ENABLED = false;
+ public static final String CONFIG_OF_OOM_PRE_QUERY_KILL_PAUSE_DURATION_MS =
+ "accounting.oom.pre.query.kill.pause.duration.ms";
+ public static final long DEFAULT_OOM_PRE_QUERY_KILL_PAUSE_DURATION_MS = -1;
+
+ public static final String CONFIG_OF_OOM_PANIC_PRE_QUERY_KILL_PAUSE_ENABLED =
+ "accounting.oom.panic.pre.query.kill.pause.enabled";
Review Comment:
(optional) I feel it might be clearer:
```suggestion
"accounting.oom.panic.allow.pre.query.kill.pause";
```
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
Review Comment:
(minor) This local variable is no longer needed
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -266,6 +313,56 @@ private void evalTriggers() {
}
}
+ /**
+ * Activates an OOM pause. Query threads calling {@link #waitIfPaused()} will block until the pause is cleared or the
+ * deadline expires. Also hints the JVM to run garbage collection.
+ */
+ private void activatePause(long timeoutMs) {
+ LOGGER.warn("Activating OOM pause for {}ms to allow garbage collection before killing queries. "
+ + "Heap used bytes: {}", timeoutMs, _usedBytes);
+ // Write deadline before the flag - volatile ordering guarantees any thread that reads
+ // _pauseActive == true will also see the updated _pauseDeadlineMs.
+ _pauseDeadlineMs = System.currentTimeMillis() + timeoutMs;
+ _pauseActive = true;
+ // Hint the JVM to run GC while query threads are pausing
+ System.gc();
+ }
+
+ /**
+ * Clears the OOM pause and wakes all blocked query threads.
+ */
+ void clearPause() {
Review Comment:
(minor) Is this exposed for testing purpose? Either annotate it or make it `private`. Same for other places
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -75,6 +78,15 @@ enum TriggeringLevel {
private Map<String, QueryResourceTrackerImpl> _queryResourceUsages;
private TriggeringLevel _triggeringLevel = TriggeringLevel.Normal;
+ // Track previous triggering level to detect transitions into critical
+ private TriggeringLevel _previousTriggeringLevel = TriggeringLevel.Normal;
+
+ // OOM pause state - lock+condition for blocking query threads at critical heap level
+ private final ReentrantLock _pauseLock = new ReentrantLock();
+ private final Condition _pauseCondition = _pauseLock.newCondition();
+ private volatile boolean _pauseActive = false;
+ private volatile long _pauseDeadlineMs = 0;
Review Comment:
(nit) Doesn't need to be volatile as it is maintained as a local state for the watcher.
Consider adding some comments describing the contract for them:
_pauseActive is shared by both watcher and query threads
_pauseDeadlineMs is local state for the watcher (similar to `_triggeringLevel`)
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3081737157
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -266,6 +302,60 @@ private void evalTriggers() {
}
}
+ /**
+ * Activates an OOM pause. Query threads calling {@link #waitIfPaused()} will block until the pause is cleared or the
+ * deadline expires. Also hints the JVM to run garbage collection.
+ */
+ private void activatePause(long timeoutMs) {
+ LOGGER.warn("Activating OOM pause for {}ms to allow garbage collection before killing queries. "
+ + "Heap used bytes: {}", timeoutMs, _usedBytes);
+ // Write deadline before the flag - volatile ordering guarantees any thread that reads
+ // _pauseActive == true will also see the updated _pauseDeadlineMs.
+ _pauseDeadlineMs = System.currentTimeMillis() + timeoutMs;
+ _pauseActive = true;
+ // Hint the JVM to run GC while query threads are pausing
+ System.gc();
+ }
+
+ /**
+ * Clears the OOM pause and wakes all blocked query threads.
+ */
+ private void clearPause() {
+ LOGGER.info("Clearing OOM pause. Heap used bytes: {}", _usedBytes);
+ _pauseLock.lock();
+ try {
+ _pauseActive = false;
+ _pauseCondition.signalAll();
+ } finally {
+ _pauseLock.unlock();
+ }
+ }
+
+ /**
+ * Called by query threads at sampling checkpoints. Blocks if an OOM pause is active. Fast-path: single volatile read
+ * when no pause is active.
+ */
+ void waitIfPaused() {
+ if (!_pauseActive) {
+ return;
+ }
+ _pauseLock.lock();
+ try {
+ while (_pauseActive) {
+ long remaining = _pauseDeadlineMs - System.currentTimeMillis();
+ if (remaining <= 0) {
+ break;
+ }
+ _pauseCondition.await(remaining, TimeUnit.MILLISECONDS);
Review Comment:
We could, but that relies solely on the watcher task thread to wake up query execution threads - this wait with timeout is like a double safeguard to ensure that query threads don't ever end up waiting indefinitely and appear stuck. Any particular issues with this wait with timeout approach?
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3089766509
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1752,6 +1746,12 @@ public static class Accounting {
public static final String CONFIG_OF_QUERY_KILLED_METRIC_ENABLED = "accounting.query.killed.metric.enabled";
public static final boolean DEFAULT_QUERY_KILLED_METRIC_ENABLED = false;
+ public static final String CONFIG_OF_OOM_PAUSE_ENABLED = "accounting.oom.critical.query.pause.enabled";
+ public static final boolean DEFAULT_OOM_PAUSE_ENABLED = false;
+
+ public static final String CONFIG_OF_OOM_PAUSE_TIMEOUT_MS = "accounting.oom.critical.query.pause.timeout.ms";
Review Comment:
`accounting.oom.pre.query.kill.pause.duration.ms` sounds good. I've also added a new boolean config `accounting.oom.panic.pre.query.kill.pause.enabled` (defaulting to false) which can control the pause on direct transition to panic (bypassing critical).
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3070389109
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/ResourceUsageAccountantFactory.java:
##########
@@ -133,6 +133,8 @@ public void sampleUsage() {
if (_memorySamplingEnabled) {
threadTracker.updateMemorySnapshot();
}
+ // Block if an OOM pause is active (fast-path: single volatile read)
+ _queryResourceAggregator.waitIfPaused();
Review Comment:
IMO this shouldn't be part of sample usage, but termination check: pause first, then check termination. Make sure termination is set before releasing pause
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -266,6 +302,60 @@ private void evalTriggers() {
}
}
+ /**
+ * Activates an OOM pause. Query threads calling {@link #waitIfPaused()} will block until the pause is cleared or the
+ * deadline expires. Also hints the JVM to run garbage collection.
+ */
+ private void activatePause(long timeoutMs) {
+ LOGGER.warn("Activating OOM pause for {}ms to allow garbage collection before killing queries. "
+ + "Heap used bytes: {}", timeoutMs, _usedBytes);
+ // Write deadline before the flag - volatile ordering guarantees any thread that reads
+ // _pauseActive == true will also see the updated _pauseDeadlineMs.
+ _pauseDeadlineMs = System.currentTimeMillis() + timeoutMs;
+ _pauseActive = true;
+ // Hint the JVM to run GC while query threads are pausing
+ System.gc();
+ }
+
+ /**
+ * Clears the OOM pause and wakes all blocked query threads.
+ */
+ private void clearPause() {
+ LOGGER.info("Clearing OOM pause. Heap used bytes: {}", _usedBytes);
+ _pauseLock.lock();
+ try {
+ _pauseActive = false;
+ _pauseCondition.signalAll();
+ } finally {
+ _pauseLock.unlock();
+ }
+ }
+
+ /**
+ * Called by query threads at sampling checkpoints. Blocks if an OOM pause is active. Fast-path: single volatile read
+ * when no pause is active.
+ */
+ void waitIfPaused() {
+ if (!_pauseActive) {
+ return;
+ }
+ _pauseLock.lock();
+ try {
+ while (_pauseActive) {
+ long remaining = _pauseDeadlineMs - System.currentTimeMillis();
+ if (remaining <= 0) {
+ break;
+ }
+ _pauseCondition.await(remaining, TimeUnit.MILLISECONDS);
Review Comment:
Do we need timeout here, or only rely on condition variable?
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -179,7 +200,22 @@ public void postAggregate() {
}
switch (_triggeringLevel) {
case HeapMemoryCritical:
- killMostExpensiveQuery();
+ QueryMonitorConfig config = _queryMonitorConfig.get();
+ if (_pauseActive) {
+ // Pause is active - check if grace period expired
+ if (System.currentTimeMillis() >= _pauseDeadlineMs) {
+ clearPause();
+ killMostExpensiveQuery();
Review Comment:
Kill first, then clear pause
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -404,6 +404,8 @@ protected void logQueryMonitorConfig(QueryMonitorConfig queryMonitorConfig) {
LOGGER.info("_minMemoryFootprintForKill: {}", queryMonitorConfig.getMinMemoryFootprintForKill());
LOGGER.info("_cpuTimeBasedKillingEnabled: {}", queryMonitorConfig.isCpuTimeBasedKillingEnabled());
LOGGER.info("_cpuTimeBasedKillingThresholdNs: {}", queryMonitorConfig.getCpuTimeBasedKillingThresholdNs());
+ LOGGER.info("_oomPauseEnabled: {}", queryMonitorConfig.isOomPauseEnabled());
Review Comment:
Is this feature available in this impl? If not, let's not log the config
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1752,6 +1746,12 @@ public static class Accounting {
public static final String CONFIG_OF_QUERY_KILLED_METRIC_ENABLED = "accounting.query.killed.metric.enabled";
public static final boolean DEFAULT_QUERY_KILLED_METRIC_ENABLED = false;
+ public static final String CONFIG_OF_OOM_PAUSE_ENABLED = "accounting.oom.critical.query.pause.enabled";
+ public static final boolean DEFAULT_OOM_PAUSE_ENABLED = false;
+
+ public static final String CONFIG_OF_OOM_PAUSE_TIMEOUT_MS = "accounting.oom.critical.query.pause.timeout.ms";
Review Comment:
We can merge them into one config, where non-positive value (e.g. default `-1`) means no pause. Something like `accounting.oom.query.kill.grace.period.ms`
Should we also allow pausing when memory usage directly jumping into panic level? Or consider using a flag to control 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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18163:
URL: https://github.com/apache/pinot/pull/18163#issuecomment-4291384440
Docs PR opened: https://github.com/pinot-contrib/pinot-docs/pull/750
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3083197169
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -266,6 +302,60 @@ private void evalTriggers() {
}
}
+ /**
+ * Activates an OOM pause. Query threads calling {@link #waitIfPaused()} will block until the pause is cleared or the
+ * deadline expires. Also hints the JVM to run garbage collection.
+ */
+ private void activatePause(long timeoutMs) {
+ LOGGER.warn("Activating OOM pause for {}ms to allow garbage collection before killing queries. "
+ + "Heap used bytes: {}", timeoutMs, _usedBytes);
+ // Write deadline before the flag - volatile ordering guarantees any thread that reads
+ // _pauseActive == true will also see the updated _pauseDeadlineMs.
+ _pauseDeadlineMs = System.currentTimeMillis() + timeoutMs;
+ _pauseActive = true;
+ // Hint the JVM to run GC while query threads are pausing
+ System.gc();
+ }
+
+ /**
+ * Clears the OOM pause and wakes all blocked query threads.
+ */
+ private void clearPause() {
+ LOGGER.info("Clearing OOM pause. Heap used bytes: {}", _usedBytes);
+ _pauseLock.lock();
+ try {
+ _pauseActive = false;
+ _pauseCondition.signalAll();
+ } finally {
+ _pauseLock.unlock();
+ }
+ }
+
+ /**
+ * Called by query threads at sampling checkpoints. Blocks if an OOM pause is active. Fast-path: single volatile read
+ * when no pause is active.
+ */
+ void waitIfPaused() {
+ if (!_pauseActive) {
+ return;
+ }
+ _pauseLock.lock();
+ try {
+ while (_pauseActive) {
+ long remaining = _pauseDeadlineMs - System.currentTimeMillis();
+ if (remaining <= 0) {
+ break;
+ }
+ _pauseCondition.await(remaining, TimeUnit.MILLISECONDS);
Review Comment:
It is a little bit risky because thread might wake up before watcher thread tries to terminate the query, causing higher memory usage
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3093056457
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -179,7 +211,22 @@ public void postAggregate() {
}
switch (_triggeringLevel) {
case HeapMemoryCritical:
- killMostExpensiveQuery();
+ QueryMonitorConfig config = _queryMonitorConfig.get();
+ if (_pauseActive) {
+ // Pause is active - check if grace period expired
+ if (System.currentTimeMillis() >= _pauseDeadlineMs) {
+ killMostExpensiveQuery();
+ clearPause();
+ }
+ // else: still in grace period, skip kill this cycle
+ } else if (config.isOomPreQueryKillPauseEnabled()
Review Comment:
This only pauses when the previous cycle was below critical. If the config is enabled while the heap is already critical/panic, the watcher still takes the immediate-kill path until usage drops below critical and rises again, so the advertised dynamic toggle does not take effect during an active incident. Please make the pause arm based on the current config/state, not only on the transition edge.
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3089864389
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -149,11 +148,23 @@ public void preAggregate(Iterable<ThreadResourceTrackerImpl> threadTrackers) {
_previousTriggeringLevel = _triggeringLevel;
collectTriggerMetrics();
evalTriggers();
- // Prioritize the panic check, kill ALL QUERIES immediately if triggered
+ // Prioritize the panic check
if (_triggeringLevel == TriggeringLevel.HeapMemoryPanic) {
- killAllQueries(threadTrackers);
+ boolean pauseOnPanic = config.isOomPauseEnabled() && config.isOomPauseOnPanicEnabled();
if (_pauseActive) {
- clearPause();
+ if (System.currentTimeMillis() >= _pauseDeadlineMs || !pauseOnPanic) {
+ // Grace period expired, or panic-pause disabled — kill all and clear
+ killAllQueries(threadTrackers);
Review Comment:
Hm actually now that I think about it, we probably don't want to wait for grace period to expire on transition from critical to panic, even if the pause on panic config is enabled..
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3067928476
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -179,7 +200,22 @@ public void postAggregate() {
}
switch (_triggeringLevel) {
case HeapMemoryCritical:
- killMostExpensiveQuery();
+ QueryMonitorConfig config = _queryMonitorConfig.get();
+ if (_pauseActive) {
+ // Pause is active - check if grace period expired
+ if (System.currentTimeMillis() >= _pauseDeadlineMs) {
+ clearPause();
Review Comment:
Clearing the pause before `killMostExpensiveQuery()` wakes every blocked query while the heap is still critical. Query termination here is cooperative (`terminate()` just sets the exception / cancels registered futures), so those threads can resume allocating again before the victim observes termination. That defeats the quiescence this pause is supposed to provide and can push the JVM straight into panic. The pause needs to stay active until after the victim query has been selected and terminated.
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #18163:
URL: https://github.com/apache/pinot/pull/18163
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3081806001
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/ResourceUsageAccountantFactory.java:
##########
@@ -133,6 +133,8 @@ public void sampleUsage() {
if (_memorySamplingEnabled) {
threadTracker.updateMemorySnapshot();
}
+ // Block if an OOM pause is active (fast-path: single volatile read)
+ _queryResourceAggregator.waitIfPaused();
Review Comment:
That will require some SPI changes, how about we swap order of sampling usage and termination checks instead (i.e., sample first - pause if needed - check termination)?
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3083160730
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/ResourceUsageAccountantFactory.java:
##########
@@ -133,6 +133,8 @@ public void sampleUsage() {
if (_memorySamplingEnabled) {
threadTracker.updateMemorySnapshot();
}
+ // Block if an OOM pause is active (fast-path: single volatile read)
+ _queryResourceAggregator.waitIfPaused();
Review Comment:
SPI change is fine. I'm thinking from an abstraction level, where there action belongs to the termination check, not usage sampling. E.g. the engine might only check termination without doing sampling
--
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 OOM pause mechanism to pause query threads before killing on critical heap [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18163:
URL: https://github.com/apache/pinot/pull/18163#discussion_r3095956023
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryThreadContext.java:
##########
@@ -259,6 +263,7 @@ public static void checkTermination(Supplier<String> scopeSupplier) {
// NOTE: In production code, threadContext should never be null. It might be null in tests when QueryThreadContext
// is not set up.
if (threadContext != null) {
+ threadContext.waitIfPausedInternal();
Review Comment:
Just realized that the second termination check should only be done if we're actually pausing and exiting the pause, and not on the regular fast path. Making this change now.
--
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