Copilot commented on code in PR #17788:
URL: https://github.com/apache/pinot/pull/17788#discussion_r2868252431
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -426,7 +426,6 @@ public void run() {
protected void runOnce() {
LOGGER.debug("Running timed task for PerQueryCPUMemAccountant.");
QueryMonitorConfig config = _queryMonitorConfig.get();
- _sleepTime = config.getNormalSleepTime();
_queryResourceUsages = null;
Review Comment:
`_sleepTime` is no longer set at the beginning of `runOnce()`. If an
exception occurs before `evalTriggers()` runs (especially on the first
iteration when `_sleepTime` is still 0), the outer loop will call
`Thread.sleep(_sleepTime)` with 0 and can busy-loop. Consider setting
`_sleepTime = config.getNormalSleepTime()` up-front as a safe default (and then
overriding it in `evalTriggers()`), or otherwise ensure `_sleepTime` always has
a sane minimum even on failures.
```suggestion
_queryResourceUsages = null;
// Ensure _sleepTime has a sane default even if an exception occurs
before evalTriggers() updates it
_sleepTime = config.getNormalSleepTime();
```
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryResourceAggregator.java:
##########
@@ -130,8 +130,6 @@ public int getAggregationSleepTimeMs() {
@Override
public void preAggregate(Iterable<ThreadResourceTrackerImpl> threadTrackers)
{
LOGGER.debug("Running pre-aggregate for QueryResourceAggregator.");
Review Comment:
`_sleepTime` is no longer initialized before calling
`collectTriggerMetrics()`/`evalTriggers()`. If `preAggregate()` throws (or is
interrupted) before `_sleepTime` is set,
`ResourceUsageAccountantFactory.WatcherTask` will reschedule immediately (sleep
time 0) and can spin in a tight loop, amplifying load when the system is
already unhealthy. Consider initializing `_sleepTime` to
`config.getNormalSleepTime()` at the start of `preAggregate()` (or in the
constructor), and then let `evalTriggers()` override it for
ALARMING/CRITICAL/PANIC.
```suggestion
LOGGER.debug("Running pre-aggregate for QueryResourceAggregator.");
_sleepTime = _queryMonitorConfig.get().getNormalSleepTime();
```
--
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]
