praveenc7 commented on code in PR #16172:
URL: https://github.com/apache/pinot/pull/16172#discussion_r2162616542
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -482,6 +482,17 @@ public Map<String, AggregatedStats> aggregate(boolean
isTriggered) {
public void postAggregation(Map<String, AggregatedStats>
aggregatedUsagePerActiveQuery) {
}
+ protected void logQueryResourceUsage(Map<String, ? extends
QueryResourceTracker> aggregatedUsagePerActiveQuery) {
+ LOGGER.warn("Current task status recorded is {}", _threadEntriesMap);
+ LOGGER.warn("Query aggregation results {} for the previous kill.",
aggregatedUsagePerActiveQuery.toString());
Review Comment:
We now emit two WARNs per kill: one line for each terminated query plus a
second line with the whole map. Does making the “aggregation results” line INFO
(or even DEBUG) help so we can keep WARN dedicated to actionable events.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -482,6 +482,17 @@ public Map<String, AggregatedStats> aggregate(boolean
isTriggered) {
public void postAggregation(Map<String, AggregatedStats>
aggregatedUsagePerActiveQuery) {
}
+ protected void logQueryResourceUsage(Map<String, ? extends
QueryResourceTracker> aggregatedUsagePerActiveQuery) {
Review Comment:
Looks like `killAllQueries()` loops over every entry in queryResources and
logs each individually. For clusters in meltdown that could still flood logs.
Do we want to cap this at some N entries?
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -482,6 +482,17 @@ public Map<String, AggregatedStats> aggregate(boolean
isTriggered) {
public void postAggregation(Map<String, AggregatedStats>
aggregatedUsagePerActiveQuery) {
}
+ protected void logQueryResourceUsage(Map<String, ? extends
QueryResourceTracker> aggregatedUsagePerActiveQuery) {
+ LOGGER.warn("Current task status recorded is {}", _threadEntriesMap);
+ LOGGER.warn("Query aggregation results {} for the previous kill.",
aggregatedUsagePerActiveQuery.toString());
+ }
+
+ protected void logTerminatedQuery(QueryResourceTracker
queryResourceTracker, long totalHeapMemoryUsage) {
Review Comment:
nit : This helper are declared protected but only used inside the same inner
class; would private would convey intent better (and allow inlining in future
clean-ups).
--
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]
