You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "hongkunxu (via GitHub)" <gi...@apache.org> on 2026/04/22 15:52:55 UTC
[PR] [bugfix] fix memory leak [pinot]
hongkunxu opened a new pull request, #18290:
URL: https://github.com/apache/pinot/pull/18290
## Background
We packaged a new `pinot-jdbc-driver` based on Pinot 1.5.0 and rolled it out
to production services that use `PinotDriver` behind a HikariCP connection
pool — a standard, high-throughput JDBC integration pattern.
Shortly after the upgrade, those services began reporting `OutOfMemoryError`
in production. The OOMs were not query-related and could not be reproduced
with ad-hoc single-connection usage; they correlated strictly with the use
of a connection pool and the amount of wall-clock time the service had been
running.
**Screenshot 1 — Production OOM stack trace from the affected service:**
<img width="920" height="614" alt="image" src="https://github.com/user-attachments/assets/2ab338b0-7c68-4991-8c58-d0e21d0e0f73" />
**Screenshot 2 — Reproduction showing thread count growing by 20 every 5
minutes under HikariCP (pool size = 20):**
<img width="1613" height="427" alt="image" src="https://github.com/user-attachments/assets/705d87ea-cce1-4721-8efb-52eb62289b27" />
**Screenshot 3 — Same reproduction after this fix; thread count stays flat
across multiple refresh cycles:**
## Problem
Each `BrokerCache` instance creates its own `AsyncHttpClient`, which in turn
creates its own private `NioEventLoopGroup` (size `2 * availableProcessors()`)
and `HashedWheelTimer` thread. When the Pinot Java / JDBC client is used
behind a connection pool (e.g. HikariCP) with N pooled connections, there are
N `BrokerCache` instances, and therefore N independent Netty thread pools for
a task that is fundamentally shared: periodically refreshing the
broker-to-table mapping from the controller.
Two behaviors compound the cost:
1. `BrokerCacheUpdaterPeriodic` refreshes every 5 minutes by default. Between
refreshes the pooled HTTP connection sits idle for ~5 minutes, well beyond
AHC's default `pooledConnectionIdleTimeout` of 60s, so the connection pool
cleaner closes the TCP connection every cycle.
2. The next refresh opens a brand-new TCP connection, which Netty binds to
the next `NioEventLoop` slot via round-robin. If that slot's worker thread
has not been lazily spawned yet, it is spawned now. This means the NIO
thread count grows by roughly +1 per `BrokerCache` per refresh interval,
until each private `NioEventLoopGroup` reaches its `2 * NPROC` cap.
With N pooled JDBC connections on an M-core host, `BrokerCache` alone can
contribute up to `N * 2 * M` Netty NIO threads plus `N` `HashedWheelTimer`
threads, growing observably over time.
## Root cause
Each `BrokerCache` owns a private Netty I/O thread pool for a workload that
is effectively homogeneous across all instances (GET `/brokers/tables` from
the controller). The per-instance ownership model is the source of the
amplification; the instances do not need private thread pools to remain
correct.
## Fix
Introduce `PinotClientNettyResources`, a small holder that exposes a single
JVM-wide daemon `NioEventLoopGroup` and a single JVM-wide daemon
`HashedWheelTimer`. `BrokerCache` now injects both into its AHC builder via
`setEventLoopGroup(...)` and `setNettyTimer(...)`.
AsyncHttpClient natively supports this sharing pattern: when either resource
is supplied externally, AHC does not shut it down on `close()`
(`ChannelManager#allowReleaseEventLoopGroup` and
`DefaultAsyncHttpClient#allowStopNettyTimer` are both set to `false` in that
case). Each `BrokerCache` still owns its own `AsyncHttpClient`, connection
pool, SSL context, timeouts, and headers — only the low-level Netty I/O
threads and timer thread are shared.
After this change, the total NIO thread count contributed by `BrokerCache`
refreshes is bounded by `2 * availableProcessors()` globally, independent of
the number of pooled `PinotConnection` / `Connection` instances. All shared
threads are daemon, so they do not block JVM exit.
## Scope of change
- New class: `pinot-clients/pinot-java-client/.../PinotClientNettyResources.java`
- Modified: `pinot-clients/pinot-java-client/.../BrokerCache.java` — two
extra builder calls (`setEventLoopGroup`, `setNettyTimer`) on the existing
AHC configuration.
No public API changes. No changes to `JsonAsyncHttpPinotClientTransport`,
`PinotControllerTransport`, `ControllerBasedBrokerSelector`, `PinotDriver`,
or `ConnectionFactory`. Behavior of direct Java-client users of
`BrokerCache` is unchanged other than the reduced thread footprint.
## Why this is safe
- **Thread safety**: `NioEventLoopGroup` and `HashedWheelTimer` are
thread-safe and explicitly designed to be shared across clients; this is
the standard pattern documented by both Netty and AsyncHttpClient.
- **Per-instance configuration preserved**: SSL context, read/connect/
handshake timeouts, user agent, enabled TLS protocols, and request
headers are all still scoped to each `BrokerCache`'s own `AsyncHttpClient`
and its own connection pool.
- **No lifecycle regression**: AHC will not shut down externally-supplied
Netty resources, so `BrokerCache.close()` continues to close its own AHC
cleanly without affecting other `BrokerCache` instances.
- **JVM exit**: the shared threads are daemon, so they do not hold the JVM
alive.
## Impact on QPS and controller load
No change. Each `BrokerCache` still issues the same number of requests at
the same cadence against the same controller endpoint. Sharing I/O threads
can only reduce context-switch overhead, never add to it.
## Testing
- Manual reproduction with HikariCP + `PinotDriver`, pool size N, on an M-core
host: before this change the NIO thread count grows by N every
`brokerUpdateFreqInMillis`; after this change it is bounded by `2 * M`
regardless of N.
- Existing `BrokerCache` / `BrokerCacheUpdaterPeriodic` unit tests continue
to pass; behavior of the constructor and `updateBrokerData()` is unchanged
from the caller's perspective.
## Follow-ups (not in this PR)
- Daemonize `BrokerCacheUpdaterPeriodic`'s `ScheduledExecutorService` thread.
- Add a bounded timeout to the blocking `Future#get()` call in
`BrokerCache#getTableToBrokersData()`.
- Optionally apply the same sharing to `JsonAsyncHttpPinotClientTransport`
and `PinotControllerTransport`.
--
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] [bugfix] fix memory leak [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #18290:
URL: https://github.com/apache/pinot/pull/18290
--
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] [bugfix] fix memory leak [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18290:
URL: https://github.com/apache/pinot/pull/18290#issuecomment-4298004725
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18290?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 55.52%. Comparing base ([`81732e0`](https://app.codecov.io/gh/apache/pinot/commit/81732e0fb7501c25e10638a34b9e6a03192424a8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e8b6810`](https://app.codecov.io/gh/apache/pinot/commit/e8b6810ce7b40a4c220022783d194a4d2a22793c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (81732e0) and HEAD (e8b6810). Click for more details.
>
> <details><summary>HEAD has 28 uploads less than BASE</summary>
>
>| Flag | BASE (81732e0) | HEAD (e8b6810) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|8|1|
>|java-11|3|0|
>|unittests2|2|0|
>|integration|4|0|
>|integration2|2|0|
>|integration1|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18290 +/- ##
============================================
- Coverage 63.65% 55.52% -8.14%
+ Complexity 1659 837 -822
============================================
Files 3244 2537 -707
Lines 197390 146139 -51251
Branches 30555 23525 -7030
============================================
- Hits 125646 81139 -44507
+ Misses 61690 58001 -3689
+ Partials 10054 6999 -3055
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.52% <ø> (-8.11%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.52% <ø> (-8.14%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.52% <ø> (-8.13%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18290/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.52% <ø> (-0.05%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18290/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/18290?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