Copilot commented on code in PR #17788:
URL: https://github.com/apache/pinot/pull/17788#discussion_r2868290293
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -477,20 +477,29 @@ protected void evalTriggers() {
// Compute the new triggering level based on the current heap usage
QueryMonitorConfig config = _queryMonitorConfig.get();
- _triggeringLevel =
- config.isCpuTimeBasedKillingEnabled() ?
TriggeringLevel.CPUTimeBasedKilling : TriggeringLevel.Normal;
if (_usedBytes > config.getPanicLevel()) {
+ // PANIC
+ _sleepTime = config.getAlarmingSleepTime();
_triggeringLevel = TriggeringLevel.HeapMemoryPanic;
_metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
} else if (_usedBytes > config.getCriticalLevel()) {
+ // CRITICAL
+ _sleepTime = config.getAlarmingSleepTime();
_triggeringLevel = TriggeringLevel.HeapMemoryCritical;
_metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
} else if (_usedBytes > config.getAlarmingLevel()) {
+ // ALARMING
_sleepTime = config.getAlarmingSleepTime();
Review Comment:
This change alters the polling interval for CRITICAL/PANIC by setting
`_sleepTime` to `getAlarmingSleepTime()`, but there are no unit tests covering
the `evalTriggers()` mapping from heap usage -> (`_triggeringLevel`,
`_sleepTime`). Adding focused tests for NORMAL/ALARMING/CRITICAL/PANIC would
help ensure the watcher loop runs at the intended frequency under memory
pressure.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -202,20 +202,28 @@ private void evalTriggers() {
// Compute the new triggering level based on the current heap usage
QueryMonitorConfig config = _queryMonitorConfig.get();
- _triggeringLevel =
- config.isCpuTimeBasedKillingEnabled() ?
TriggeringLevel.CPUTimeBasedKilling : TriggeringLevel.Normal;
if (_usedBytes > config.getPanicLevel()) {
+ // PANIC
+ _sleepTime = config.getAlarmingSleepTime();
_triggeringLevel = TriggeringLevel.HeapMemoryPanic;
_metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
} else if (_usedBytes > config.getCriticalLevel()) {
+ // CRITICAL
+ _sleepTime = config.getAlarmingSleepTime();
_triggeringLevel = TriggeringLevel.HeapMemoryCritical;
_metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
} else if (_usedBytes > config.getAlarmingLevel()) {
+ // ALARMING
_sleepTime = config.getAlarmingSleepTime();
- // For debugging
- if (LOGGER.isDebugEnabled() && _triggeringLevel ==
TriggeringLevel.Normal) {
- _triggeringLevel = TriggeringLevel.HeapMemoryAlarmingVerbose;
+ if (config.isCpuTimeBasedKillingEnabled()) {
+ _triggeringLevel = TriggeringLevel.CPUTimeBasedKilling;
+ } else {
+ _triggeringLevel = LOGGER.isDebugEnabled() ?
TriggeringLevel.HeapMemoryAlarmingVerbose : TriggeringLevel.Normal;
Review Comment:
The `LOGGER.isDebugEnabled() ? ... : ...` assignment is long and hard to
scan on a single line. Consider formatting it similarly to the equivalent code
in `PerQueryCPUMemAccountantFactory` (line break) or using an explicit
`if/else` to keep this block readable.
```suggestion
if (LOGGER.isDebugEnabled()) {
_triggeringLevel = TriggeringLevel.HeapMemoryAlarmingVerbose;
} else {
_triggeringLevel = TriggeringLevel.Normal;
}
```
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -202,20 +202,28 @@ private void evalTriggers() {
// Compute the new triggering level based on the current heap usage
QueryMonitorConfig config = _queryMonitorConfig.get();
- _triggeringLevel =
- config.isCpuTimeBasedKillingEnabled() ?
TriggeringLevel.CPUTimeBasedKilling : TriggeringLevel.Normal;
if (_usedBytes > config.getPanicLevel()) {
+ // PANIC
+ _sleepTime = config.getAlarmingSleepTime();
_triggeringLevel = TriggeringLevel.HeapMemoryPanic;
_metrics.addMeteredGlobalValue(_heapMemoryPanicExceededMeter, 1);
} else if (_usedBytes > config.getCriticalLevel()) {
+ // CRITICAL
+ _sleepTime = config.getAlarmingSleepTime();
_triggeringLevel = TriggeringLevel.HeapMemoryCritical;
_metrics.addMeteredGlobalValue(_heapMemoryCriticalExceededMeter, 1);
} else if (_usedBytes > config.getAlarmingLevel()) {
+ // ALARMING
_sleepTime = config.getAlarmingSleepTime();
Review Comment:
The sleep-time behavior is now different for CRITICAL/PANIC (switching to
`getAlarmingSleepTime()`), but there are no unit tests asserting the chosen
`_sleepTime` and `_triggeringLevel` for the different heap-usage thresholds.
Please add tests that cover NORMAL/ALARMING/CRITICAL/PANIC and verify
`_sleepTime` selection (normal vs alarming) to prevent regressions.
--
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]
