You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/09/04 22:44:14 UTC
[PR] Stop a lingering receiving mailbox from pinning the receive operator [pinot]
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: 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
Re: [PR] Stop a lingering receiving mailbox from pinning the receive operator [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19465:
URL: https://github.com/apache/pinot/pull/19465#issuecomment-5547592972
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19465?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`b916e54`](https://app.codecov.io/gh/apache/pinot/commit/b916e5474c95fa07b20ee385d8067677c285e01f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`22371f4`](https://app.codecov.io/gh/apache/pinot/commit/22371f4738bdd27d4211c73dcf70bd33d3b72151?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19465 +/- ##
==============================================
+ Coverage 57.78% 100.00% +42.21%
+ Complexity 7 6 -1
==============================================
Files 2689 3 -2686
Lines 164215 6 -164209
Branches 26664 0 -26664
==============================================
- Hits 94894 6 -94888
+ Misses 61293 0 -61293
+ Partials 8028 0 -8028
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.21%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.21%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19465/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19465?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Stop a lingering receiving mailbox from pinning the receive operator [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #19465:
URL: https://github.com/apache/pinot/pull/19465
--
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