apache / apache/pulsar

[Bug] Shared MemoryLimitController silently disables the client memory limit and drops the client memory metrics

Open
#26,345 2 comments 0 reactions 1 assignee Claimed by @lhotari View on GitHub
Dominant language
Java
Stars
15.3k
Forks
3.8k
Avg merge
1d 14h
Merged PRs (30d)
160

Description

### Search before asking

- [x] I searched the [issues](https://github.com/apache/pulsar/issues) and found nothing similar. The *feature* — sharing a `MemoryLimitController` across `PulsarClient` instances — already exists (#25212, closed by #25477). This issue is about defects in that shipped feature.

### Problem

`PulsarClientSharedResources` lets several `PulsarClient` instances share one `MemoryLimitController` (PIP-234, #25477). Three defects make it a footgun rather than a safety mechanism:

**A. Sharing resources silently disables the memory limit.** The shared limit defaults to `0` = unlimited, and an unconfigured shared-resources instance shares *everything*, including the memory limit controller. So the documented "share all resources" snippet silently removes the 64 MiB default limit from every client using it — in exactly the many-clients scenario where a bound matters most.

**B. Per-client `ClientBuilder.memoryLimit(...)` is silently ignored when a controller is injected.** No warning, no exception, no validation. Two clients declaring different limits while sharing: both values are discarded.

**C. The two client memory metrics disappear, or land on the wrong meter provider.**

### Concrete evidence

**A —** `PulsarClientSharedResourcesBuilderImpl.java:209-210`:

```java
static class MemoryLimitResourceConfig implements ResourceConfig, MemoryLimitConfig {
long memoryLimit; // defaults to 0
```

`0` means unlimited (`MemoryLimitController.isMemoryLimited()` → `memoryLimit > 0`, `MemoryLimitController.java:153-155`), and an unconfigured builder shares every resource type (`PulsarClientSharedResourcesImpl.java:70-71`, `EnumSet.allOf(SharedResource.class)`), which includes `SharedResource.MemoryLimitController` (`:103-106`). The injected controller then wins unconditionally — `PulsarClientImpl.java:347-354`:

```java
if (memoryLimitController == null) {
this.memoryLimitController = new MemoryLimitController(conf.getMemoryLimitBytes(), ...);
} else {
this.memoryLimitController = memoryLimitController; // conf.getMemoryLimitBytes() never read
this.memoryLimitController.registerTrigger(this.memoryLimitTrigger);
}
```

So `ClientConfigurationData.java:436` (`memoryLimitBytes = 64 * 1024 * 1024`) is discarded. **The affected snippet is the documented usage example** in `PulsarClientSharedResources.java` ("To share all possible resources across multiple PulsarClient instances" → `builder().build()`), and `PulsarClientSharedResourcesBuilderImplTest` exercises exactly that shape across 1000 clients while asserting nothing about the limit — `rg 'MemoryLimit|memoryLimit'` over that test file returns **zero** matches.

**B —** same `PulsarClientImpl.java:347-354`. Neither `ClientBuilder.memoryLimit(...)` nor `ClientBuilder.sharedResources(...)` javadoc mentions the interaction, and nothing validates a conflict.

**C —** `PulsarClientImpl.java:356-360`:

```java
// Only create memory buffer metrics if memory limit controller is local and memory limiting is enabled.
if (memoryLimitController == null && this.memoryLimitController.isMemoryLimited()) {
this.memoryBufferStats = new MemoryBufferStats(this.instrumentProvider, this.memoryLimitController);
} else {
this.memoryBufferStats = null;
}
```

The condition keys off the **injected constructor parameter**, so any shared controller suppresses the per-client registration of `pulsar.client.memory.buffer.usage` and `pulsar.client.memory.buffer.limit` (`metrics/MemoryBufferStats.java:26-30`). The compensating registration in `PulsarClientSharedResourcesImpl.java:112-117` requires **both** a non-zero shared limit **and** `SharedResource.OpenTelemetry` in the shared set. Outcomes:

- Shared controller left at its `0` default → neither side registers → **both metrics silently vanish.**
- `SharedResource.OpenTelemetry` not shared (e.g. an explicit resource list, or `shareConfigured()` with only `configureMemoryLimitController(...)`) → `instrumentProvider == null` (`:107-110`) → **both metrics silently vanish**, even with a non-zero shared limit.
- Everything shared, limit configured, but OTel unconfigured → the shared `InstrumentProvider` falls back to `GlobalOpenTelemetry.get()` (`InstrumentProvider.java:38-43`), so these two metrics go to the global instance while **every other client metric** goes to the SDK passed to `ClientBuilder.openTelemetry(sdk)`.

The last case is an API gap, not a missing line. `PulsarClientSharedResourcesImpl.applyTo` (`:168-193`) sets 8 things and `instrumentProvider` is not among them, and it *cannot* be: the Lombok `@Builder` sits on the `PulsarClientImpl` constructor, whose parameter list has no `instrumentProvider`, so `PulsarClientImplBuilder` has no such setter. `PulsarClientImpl.java:275` unconditionally does `new InstrumentProvider(conf.getOpenTelemetry())`. Consequently `SharedResource.OpenTelemetry` affects **only** the shared `memoryBufferStats` and nothing else — and repo-wide, `configureOpenTelemetry(...)` and `PulsarClientSharedResourcesImpl.getInstrumentProvider()` have **zero callers**, so nothing today depends on the current behaviour.

`MemoryLimitConfig`'s javadoc reinforces the mismatch: it says "See also `ClientBuilder#memoryLimit(long, SizeUnit)`", reading as a shared analogue of the client-level setting, while in fact it silently overrides it and only its non-zero form keeps the metrics alive.

### Reachability

No broker or network needed. `PulsarClientSharedResources.builder().build()` + `PulsarClient.builder().sharedResources(shared)` is sufficient to observe A and C; assert on `((PulsarClientImpl) client).getMemoryLimitController().isMemoryLimited()` and on the absence of the two metrics. Existing coverage (`ProducerMemoryLimitTest.testMultiPulsarClientProducerShareMemoryLimitController`, `ConsumerMemoryLimitTest.testMultiPulsarClientConsumerShareMemoryLimitController`) only covers the *configured, non-zero* path.

### Proposed solution

**For A** — pick one; this needs a call:

1. Default the shared limit to `ClientConfigurationData`'s 64 MiB instead of `0`. Consistent with the per-client default; changes behaviour for anyone relying on today's accidental "unlimited".
2. Require an explicit `configureMemoryLimitController(...)` whenever `SharedResource.MemoryLimitController` is in the shared set, and fail the build otherwise. Loudest and safest; breaks the documented `builder().build()` example.
3. Exclude `MemoryLimitController` from the share-everything default so unconfigured sharing leaves each client's own limit intact. Least disruptive, but makes "all" not actually mean all.

**For B** — reject or warn on a conflict: if a client sets `memoryLimit(...)` explicitly *and* is given a shared controller, log a warning (or fail the build) instead of discarding the value silently. Document the precedence on both `ClientBuilder.memoryLimit` and `ClientBuilder.sharedResources`.

**For C** — register the two metrics exactly once regardless of the sharing shape, and route them through the same `InstrumentProvider` as the rest of the client's metrics. The straightforward version adds `instrumentProvider` to `PulsarClientImpl`'s builder and passes the shared one through `applyTo` — a (package-private) API change, hence a design decision rather than a patch. A narrower fix: register `MemoryBufferStats` on the shared object whenever the shared controller is memory-limited, independent of whether `SharedResource.OpenTelemetry` is shared.

Fixing A is a prerequisite for the broker/proxy work in #26346: wiring the broker onto a shared controller while that controller silently defaults to unlimited would achieve nothing.

### Scope & compatibility

- **A** changes observable behaviour for anyone already sharing resources without configuring a limit. Framed as a bug fix (the current behaviour silently discards a documented default), but options 1 and 2 cross into "semantics of existing functionality" — flag in release notes; a PIP is arguably warranted for option 2, since it makes an existing documented snippet throw.
- **B** is validation plus javadoc — bug-fix scope.
- **C** is a bug fix if it only restores the two existing metric names on the shared path; it needs a **PIP** if it adds public API (propagating a shared `InstrumentProvider` into clients).
- No wire-protocol, metadata-format or client-server compatibility impact. The two metric names are unchanged in every option.

### Related

- #25212 (closed, completed) / #25477 (merged) — PIP-234 shared `MemoryLimitController`; this issue reports defects in that feature, not the feature itself.
- #19074 — PIP-234 umbrella; #24790 — shared thread pools and DNS resolver; #24893 / #24796 — shared resources in `PulsarAdmin`; #25072 / #24795 — shared resources in `AuthenticationOAuth2`.
- #18938, #18939 — memory-limit-controller accounting leaks on consumer close / `clearIncomingMessages` (different bug, non-overlapping).
- #26340 / #26341 / #26342 — the client memory-limit default work that surfaced this area.
- #26346 — configurable memory limit for broker, proxy and WebSocket proxy clients (depends on this).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.