[GitHub] [pinot] jasperjiaguo commented on a diff in pull request #10373: [Query Killing] Enhance gc related logic, refactor logging

["jasperjiaguo (via GitHub)" <gi...@apache.org>]

jasperjiaguo commented on code in PR #10373:
URL: https://github.com/apache/pinot/pull/10373#discussion_r1124924913


##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java:
##########
@@ -724,14 +745,14 @@ void killAllQueries() {
        */
       private void killMostExpensiveQuery() {
         if (!_aggregatedUsagePerActiveQuery.isEmpty() && _numQueriesKilledConsecutively >= _gcTriggerCount) {
-          System.gc();
           _numQueriesKilledConsecutively = 0;
+          System.gc();
           try {
-            Thread.sleep(_normalSleepTime);
+            Thread.sleep(_gcWaitTime);
           } catch (InterruptedException ignored) {
           }
           _usedBytes = MEMORY_MX_BEAN.getHeapMemoryUsage().getUsed();
-          if (_usedBytes < _criticalLevel) {
+          if (_usedBytes < _criticalLevelAfterGC) {

Review Comment:
   Yes that is expected to prevent heap size oscillation and repetitive gc triggering. So in production this code path is almost never triggered as `_numQueriesKilledConsecutively` is mostly at a low level and `_numQueriesKilledConsecutively >= _gcTriggerCount` is false. We now want to also use this path to "give gc a second chance" if it hasn't kick in by setting _gcTriggerCount to 0. For these use cases, even HeapMemoryCritical event is very seldom, so when it happens we want to make sure it first to a gc. What we observed there is once the gc happens it would collect a ton of garbage. So if it fails to do so (`_usedBytes > _criticalLevelAfterGC`) we should still kill the query. Nevertheless, this _criticalLevelAfterGC itself is configurable so we can make `_criticalLevelAfterGC= _criticalLevel` if we want the detection to be even less sensitive.



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org