xiangfu0 commented on code in PR #18928:
URL: https://github.com/apache/pinot/pull/18928#discussion_r3533338107
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -257,7 +257,9 @@ public void onFailure(Throwable t) {
// RejectedExecutionException. When it does, the task never runs and the
directExecutor FutureCallback above
// (which decrements the active-opchain counter and removes the
per-request context entry) never fires. Back
// out that bookkeeping here so the entry — and the
QueryExecutionContext it pins — does not leak until a later
- // cancel. Then rethrow so the caller propagates the failure as a stage
error.
+ // cancel. Then rethrow so the caller propagates the failure as a stage
error. (The caller,
+ // QueryRunner#processQueryBlocking, close()s the op chain on this
rethrow, releasing operator resources that
+ // the never-fired FutureCallback would otherwise have closed.)
Review Comment:
This rejection path also needs to invalidate `_opChainCache` for this
`opChainId`. `registerInternal()` already put the root operator and
`QueryExecutionContext` into the stats cache before `submit()`, and when
`submit()` rejects the `FutureCallback` never runs to invalidate that entry.
The caller-side `opChain.close()` releases operator resources, but the closed
operator tree and query context can still stay pinned until cache expiry under
load shedding, so this path still leaks memory on the failure mode this PR is
fixing.
--
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]
