Copilot commented on code in PR #17788:
URL: https://github.com/apache/pinot/pull/17788#discussion_r2868203736
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -479,17 +479,19 @@ protected void evalTriggers() {
QueryMonitorConfig config = _queryMonitorConfig.get();
_triggeringLevel =
config.isCpuTimeBasedKillingEnabled() ?
TriggeringLevel.CPUTimeBasedKilling : TriggeringLevel.Normal;
- if (_usedBytes > config.getPanicLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
- _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
- } else if (_usedBytes > config.getCriticalLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
- _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
- } else if (_usedBytes > config.getAlarmingLevel()) {
+ if (_usedBytes > config.getAlarmingLevel()) {
_sleepTime = config.getAlarmingSleepTime();
- // For debugging
- if (LOGGER.isDebugEnabled() && _triggeringLevel ==
TriggeringLevel.Normal) {
- _triggeringLevel = TriggeringLevel.HeapMemoryAlarmingVerbose;
+ if (_usedBytes > config.getPanicLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
+ _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
+ } else if (_usedBytes > config.getCriticalLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
+ _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter,
1);
+ } else {
Review Comment:
`evalTriggers()` now checks `alarmingLevel` first and only evaluates
CRITICAL/PANIC inside that block. This makes the triggering behavior depend on
`alarmingLevel` being lower than the other thresholds, and also contradicts the
method comment about evaluating triggers high -> low. Consider reordering back
to panic -> critical -> alarming, while still applying `alarmingSleepTime` in
the critical/panic branches.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -204,17 +204,19 @@ private void evalTriggers() {
QueryMonitorConfig config = _queryMonitorConfig.get();
_triggeringLevel =
config.isCpuTimeBasedKillingEnabled() ?
TriggeringLevel.CPUTimeBasedKilling : TriggeringLevel.Normal;
- if (_usedBytes > config.getPanicLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
- _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
- } else if (_usedBytes > config.getCriticalLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
- _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
- } else if (_usedBytes > config.getAlarmingLevel()) {
+ if (_usedBytes > config.getAlarmingLevel()) {
_sleepTime = config.getAlarmingSleepTime();
- // For debugging
- if (LOGGER.isDebugEnabled() && _triggeringLevel ==
TriggeringLevel.Normal) {
- _triggeringLevel = TriggeringLevel.HeapMemoryAlarmingVerbose;
+ if (_usedBytes > config.getPanicLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
+ _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
+ } else if (_usedBytes > config.getCriticalLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
+ _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
+ } else {
Review Comment:
The panic/critical checks are now gated by `if (_usedBytes >
config.getAlarmingLevel())`. If `alarmingLevel` is misconfigured above
`criticalLevel`/`panicLevel`, this method will no longer enter the block and
will fail to trigger CRITICAL/PANIC actions even when their thresholds are
exceeded. Consider restoring high->low evaluation (panic -> critical ->
alarming) and setting `_sleepTime = config.getAlarmingSleepTime()` in the
critical/panic branches as well, so triggers don’t depend on `alarmingLevel`
ordering.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -479,17 +479,19 @@ protected void evalTriggers() {
QueryMonitorConfig config = _queryMonitorConfig.get();
_triggeringLevel =
config.isCpuTimeBasedKillingEnabled() ?
TriggeringLevel.CPUTimeBasedKilling : TriggeringLevel.Normal;
- if (_usedBytes > config.getPanicLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
- _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
- } else if (_usedBytes > config.getCriticalLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
- _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
- } else if (_usedBytes > config.getAlarmingLevel()) {
+ if (_usedBytes > config.getAlarmingLevel()) {
_sleepTime = config.getAlarmingSleepTime();
- // For debugging
- if (LOGGER.isDebugEnabled() && _triggeringLevel ==
TriggeringLevel.Normal) {
- _triggeringLevel = TriggeringLevel.HeapMemoryAlarmingVerbose;
+ if (_usedBytes > config.getPanicLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
+ _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
+ } else if (_usedBytes > config.getCriticalLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
+ _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter,
1);
Review Comment:
This change alters the scheduling behavior by applying `alarmingSleepTime`
when heap usage is CRITICAL/PANIC. Please add/extend unit tests to cover the
updated trigger evaluation, e.g., asserting `_sleepTime` switches to
`getAlarmingSleepTime()` for CRITICAL and PANIC, and that triggering still
works when thresholds are not strictly ordered.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -204,17 +204,19 @@ private void evalTriggers() {
QueryMonitorConfig config = _queryMonitorConfig.get();
_triggeringLevel =
config.isCpuTimeBasedKillingEnabled() ?
TriggeringLevel.CPUTimeBasedKilling : TriggeringLevel.Normal;
- if (_usedBytes > config.getPanicLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
- _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
- } else if (_usedBytes > config.getCriticalLevel()) {
- _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
- _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
- } else if (_usedBytes > config.getAlarmingLevel()) {
+ if (_usedBytes > config.getAlarmingLevel()) {
_sleepTime = config.getAlarmingSleepTime();
- // For debugging
- if (LOGGER.isDebugEnabled() && _triggeringLevel ==
TriggeringLevel.Normal) {
- _triggeringLevel = TriggeringLevel.HeapMemoryAlarmingVerbose;
+ if (_usedBytes > config.getPanicLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryPanic;
+ _metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
+ } else if (_usedBytes > config.getCriticalLevel()) {
+ _triggeringLevel = TriggeringLevel.HeapMemoryCritical;
+ _metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
Review Comment:
This change modifies trigger evaluation and sleep-time behavior for
CRITICAL/PANIC conditions. Consider adding a unit test (or extending existing
accounting tests) that exercises `QueryResourceAggregator` with `_usedBytes`
above critical/panic and asserts the returned `getAggregationSleepTimeMs()`
uses `getAlarmingSleepTime()` while still triggering the expected level.
--
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]
