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-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

Reply via email to