Copilot commented on code in PR #16642:
URL: https://github.com/apache/pinot/pull/16642#discussion_r2288920633
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -631,14 +631,14 @@ public AggregatedStats merge(long cpuNS, long
memoryBytes, boolean isAnchorThrea
/**
* A watcher task to perform usage sampling, aggregation, and query
preemption
*/
Review Comment:
The @SuppressWarnings annotation is added without clear justification.
Consider either fixing the underlying type safety issues or adding a comment
explaining why these warnings need to be suppressed.
```suggestion
*/
// Suppress rawtypes and unchecked warnings due to unavoidable use of
legacy APIs or generic type erasure
// in the implementation of WatcherTask. All usages have been reviewed
for type safety.
```
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -730,32 +730,38 @@ private void logQueryMonitorConfig() {
@Override
public void run() {
- while (!Thread.currentThread().isInterrupted()) {
- try {
- runOnce();
- } finally {
- // Sleep for sometime
- reschedule();
+ try {
+ //noinspection InfiniteLoopStatement
+ while (true) {
+ try {
+ runOnce();
+ } finally {
+ //noinspection BusyWait
Review Comment:
[nitpick] Similar to the infinite loop comment, this IDE-specific
suppression comment should be replaced with a more standard documentation
comment explaining why the busy wait pattern is acceptable here.
```suggestion
// Sleep between iterations to avoid excessive resource usage.
// This is intentional and acceptable for a background watcher
task.
```
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -730,32 +730,38 @@ private void logQueryMonitorConfig() {
@Override
public void run() {
- while (!Thread.currentThread().isInterrupted()) {
- try {
- runOnce();
- } finally {
- // Sleep for sometime
- reschedule();
+ try {
+ //noinspection InfiniteLoopStatement
Review Comment:
[nitpick] The infinite loop is intentional but the comment style suggests
IDE-specific suppression. Consider using a more standard comment format that
explains the intentional infinite loop behavior for better code documentation.
```suggestion
// Intentionally use an infinite loop to continuously monitor and
update query metrics.
```
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
