Copilot commented on code in PR #16479:
URL: https://github.com/apache/pinot/pull/16479#discussion_r2246553677
##########
pinot-spi/src/main/java/org/apache/pinot/spi/accounting/ThreadResourceUsageProvider.java:
##########
@@ -51,14 +51,12 @@ public class ThreadResourceUsageProvider {
private static boolean _isThreadCpuTimeMeasurementEnabled = false;
private static boolean _isThreadMemoryMeasurementEnabled = false;
- @Deprecated
- public long getThreadTimeNs() {
- return 0;
+ public static int getThreadCount() {
Review Comment:
The new methods getThreadCount() and getTotalStartedThreadCount() replace
deprecated functionality but change from instance methods to static methods.
This could break existing code that was calling these as instance methods,
creating an incompatible API change.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/ResourceAggregator.java:
##########
@@ -21,39 +21,35 @@
import java.util.List;
-/**
- * Interface for aggregating CPU and memory usage of threads.
- */
+/// Interface for aggregating CPU and memory usage of threads.
public interface ResourceAggregator {
- /**
- * Update CPU usage for one-off cases where identifier is known before-hand.
For example: broker inbound netty
- * thread where queryId and workloadName are already known.
- *
- * @param name identifier name - workload name, queryId, etc.
- * @param cpuTimeNs CPU time in nanoseconds
- */
- public void updateConcurrentCpuUsage(String name, long cpuTimeNs);
+ /// Updates CPU usage for one-off cases where identifier is known
beforehand. For example: broker inbound netty thread
+ /// where queryId and workloadName are already known.
+ ///
+ /// @param name identifier name - queryId, workload name, etc.
+ /// @param cpuTimeNs CPU time in nanoseconds
+ void updateConcurrentCpuUsage(String name, long cpuTimeNs);
- /**
- * Update CPU usage for one-off cases where identifier is known before-hand.
For example: broker inbound netty
- * @param name identifier name - workload name, queryId, etc.
- * @param memBytes memory usage in bytes
- */
- public void updateConcurrentMemUsage(String name, long memBytes);
+ /// Updates memory usage for one-off cases where identifier is known
beforehand. For example: broker inbound netty
+ /// thread where queryId and workloadName are already known.
+ ///
+ /// @param name identifier name - queryId, workload name, etc.
+ /// @param memBytes memory usage in bytes
+ void updateConcurrentMemUsage(String name, long memBytes);
- // Cleanup of state after periodic aggregation is complete.
- public void cleanUpPostAggregation();
+ /// Cleans up state after periodic aggregation is complete.
+ void cleanUpPostAggregation();
- // Sleep time between aggregations.
- public int getAggregationSleepTimeMs();
+ /// Sleep time between aggregations.
+ int getAggregationSleepTimeMs();
- // Pre-aggregation step to be called before the aggregation of all thread
entries.
- public void
preAggregate(List<CPUMemThreadLevelAccountingObjects.ThreadEntry>
anchorThreadEntries);
+ /// Pre-aggregation step to be called before the aggregation of all thread
entries.
+ void preAggregate(List<CPUMemThreadLevelAccountingObjects.ThreadEntry>
anchorThreadEntries);
- // Aggregation of each thread entry
- public void aggregate(Thread thread,
CPUMemThreadLevelAccountingObjects.ThreadEntry threadEntry);
+ /// Aggregates on a thread entry.
+ void aggregate(Thread thread, CPUMemThreadLevelAccountingObjects.ThreadEntry
threadEntry);
- // Post-aggregation step to be called after the aggregation of all thread
entries.
- public void postAggregate();
+ /// Post-aggregation step to be called after the aggregation of all thread
entries.
Review Comment:
The documentation style has been changed from standard Javadoc (/** ... */)
to triple-slash comments (/// ...). This is inconsistent with Java
documentation conventions and may not be properly recognized by documentation
generation tools.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -559,8 +525,8 @@ public void reapFinishedTasks() {
Thread thread = entry.getKey();
if (!thread.isAlive()) {
+ LOGGER.debug("Thread: {} is no longer alive, removing it from
_threadEntriesMap", thread.getName());
_threadEntriesMap.remove(thread);
Review Comment:
The debug log message was reordered to appear before the removal operation
rather than after. While this change improves clarity, it represents a subtle
behavioral change in logging that may affect debugging workflows.
```suggestion
_threadEntriesMap.remove(thread);
LOGGER.debug("Thread: {} is no longer alive, removed it from
_threadEntriesMap", thread.getName());
```
--
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]
