vrajat commented on code in PR #16378:
URL: https://github.com/apache/pinot/pull/16378#discussion_r2214884999
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -291,6 +326,39 @@ public boolean throttleQuerySubmission() {
return getWatcherTask().getHeapUsageBytes() >
getWatcherTask().getQueryMonitorConfig().getAlarmingLevel();
}
+ public void checkMemoryAndInterruptIfExceeded() {
Review Comment:
Instead of interrupting the anchor thread, this function can be called from
`Tracing.ThreadAccountantOps.isInterrupted()`. Specifically:
```
return Tracing.getThreadAccountant.checkMemoryAndInterruptIfExceeded() ||
Thread.interrupted() ||
Tracing.getThreadAccountant().isAnchorThreadInterrupted();
```
A better long term solution is to change
``Tracing.ThreadAccountantOps.isInterrupted()` to
```
return Thread.interrupted() ||
Tracing.getThreadAccountant().isQueryTerminated();
```
`ThreadResourceUsageAccountant.isQueryTerminated()` is a new function:
```
return checkMemoryAndInterruptIfExceeded() || isAnchorThreadInterrupted()
```
The advantage is that new conditions can be added depending on the
capability of the accountant such as also interrupting in panic mode.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1529,6 +1529,15 @@ public static class Accounting {
"accounting.min.memory.footprint.to.kill.ratio";
public static final double DEFAULT_MEMORY_FOOTPRINT_TO_KILL_RATIO = 0.025;
+ // Per-query memory threshold configurations
+ public static final String CONFIG_OF_ENABLE_PER_QUERY_MEMORY_CHECK =
"accounting.per.query.memory.check.enabled";
+ public static final boolean DEFAULT_ENABLE_PER_QUERY_MEMORY_CHECK = false;
+
+ public static final String CONFIG_OF_PER_QUERY_MEMORY_LIMIT_BYTES =
"accounting.per.query.memory.limit.bytes";
Review Comment:
Should there be `thread` in the name as single thread's allocation is
checked ? Or improve the capability later to consider aggregate ?
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -155,6 +159,20 @@ protected
PerQueryCPUMemResourceUsageAccountant(PinotConfiguration config, boole
_isThreadCPUSamplingEnabled = isThreadCPUSamplingEnabled;
_isThreadMemorySamplingEnabled = isThreadMemorySamplingEnabled;
_isThreadSamplingEnabledForMSE = isThreadSamplingEnabledForMSE;
+ _isPerQueryMemoryCheckEnabled = config.getProperty(
Review Comment:
If you add these config params to `QueryMonitorConfig` - they can be changed
at runtime.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
