yashmayya opened a new pull request, #19465:
URL: https://github.com/apache/pinot/pull/19465
## The bug
A `ReceivingMailbox` outlives the query that used it, and until now it kept
the receive operator alive with it.
`MailboxService` caches receiving mailboxes with a 5 minute
`expireAfterAccess`. A mailbox is invalidated earlier only on a **successful**
EOS (`ReadMailboxAsyncStream#poll`), so after an error or a cancellation it
stays in that cache until it expires. The mailbox holds the reader callback
registered on it, which reaches the `BlockingMultiStreamConsumer`, which holds
the `ReadMailboxAsyncStream` — and that stream held a reference back to the
`BaseMailboxReceiveOperator`.
So every failed query pinned its receive operator, its
`OpChainExecutionContext`, and the `PipelineBreakerResult` blocks hanging off
that context, for up to five minutes after the query was already gone. With
stream-stats reporting enabled the context also maps every operator in the
chain (`OpChainExecutionContext#_operatorToPlanNodes`), so a single retained
receive operator pins the **whole operator tree** — every buffered row and hash
table in the stage.
The retention chain, before this change:
```
MailboxService._receivingMailboxCache (expireAfterAccess 5 min)
└─ ReceivingMailbox._blocks._reader (never cleared)
└─ BlockingMultiStreamConsumer::onData
└─ _mailboxes → ReadMailboxAsyncStream._operator ← the link
this PR removes
└─ BaseMailboxReceiveOperator._context
└─ (stream-stats mode) every operator in the stage
```
## The fix
`ReadMailboxAsyncStream` held the operator for exactly one reason: to reach
`_operator._mailboxService` in `poll()`. It now holds the `MailboxService`
directly, so the chain above stops at the stream.
The mailbox itself is still released only on a successful EOS, which is
deliberate rather than an oversight — the `TODO` that asked the question is
replaced with the answer. After an error the senders may still be running, and
they have to find the cancelled mailbox rather than recreate a fresh one that
nobody will ever read. Letting the cache expiry handle it is correct; what was
wrong is how much it dragged along while it waited.
## Testing
`shouldNotStayReachableFromTheMailboxAfterAnError` drives a real
`ReceivingMailbox` (not a mock) to an error block, closes the operator, drops
the last strong reference to it, and waits on a `ReferenceQueue` for a
`WeakReference` to clear — while keeping the mailbox strongly reachable, since
it is the retention path under test.
It fails on master and passes with this change.
Full `pinot-query-runtime` suite: 4601 tests, 0 failures, 0 errors.
`spotless`, `checkstyle` and `license` clean.
## Notes
Found while reviewing #19462, which fixes the neighbouring problem —
operators that keep their buffers after a failed query. That one is about what
an operator holds; this is about what holds the operator. They are independent,
so this goes on its own.
## Labels
`bug`, `multi-stage`
--
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]
