Repository navigation
[improve][broker] Track in-progress request frame memory - #27110
poorbarcode wants to merge 13 commits into
Conversation
Assisted-by: Codex
codelipenghui
left a comment
There was a problem hiding this comment.
Rechecked the outstanding findings against head 346ec2490abbd78978c3d8646638cacff54cc6b0.
The new shared root-reference accounting, capacity reconciliation, and latest-state notifications address the earlier pooled-buffer, growth, and stale-notification reports by source inspection; those comments are omitted.
Requesting changes for two remaining issues:
- Memory backpressure can prevent already-buffered frames from draining, even though those frames retain the allocations needed to clear the throttle.
- Concurrent cache creation can overwrite a newer dynamic cache-copy setting with the old value.
Validation scope: 18 supplied focused tests passed on the previously reviewed head 8d750c0afe9ef95d271ea11ee0000f2dc0949ebe. The queued-frame stall was reproduced there using the actual decoder, flow-control handler and throttle tracker with a mocked connection; the original-decoder control passed. The latest revision was source-rechecked only, not retested locally. The cache race is based on the concrete interleaving described inline.
| if (throttled) { | ||
| serverCnx.getThrottleTracker().markThrottled( | ||
| ServerCnxThrottleTracker.ThrottleType.InProgressRequestFrameMemoryExceeded); |
There was a problem hiding this comment.
[P1] Preserve progress for already-buffered frames under memory backpressure
This throttle disables auto-read, which also prevents PulsarFlowControlHandler from draining its existing queue. A receive allocation can contain frames A and B: if another throttle (for example, TopicPublishRate) stops delivery after A, B stays queued and retains the allocation. Once the scheduled memory-throttle update runs, releasing A and clearing the original throttle cannot resume delivery: B keeps retained memory above the resume watermark, while B cannot be processed until the memory throttle clears.
Deferring the update until after the current inbound callback does not solve this, because another throttle can leave that callback's decoded frames queued. This was reproduced on the earlier revision with the supported unpooled allocator; the current controller still enforces the same circular dependency.
Please separate stopping additional socket intake from draining already-accounted frames, or otherwise guarantee progress for the work retaining those allocations. Add a regression combining memory pressure, another throttle reason, and a queued tail sharing the receive allocation.
There was a problem hiding this comment.
The queued-frame progress fix is now included in bd7903c4e3, with the combined pooled-sibling/Topic/memory-throttle regression. Socket intake and queued-message delivery are controlled separately, while per-message business throttling is preserved. Details are in the updated follow-up reply; this fix no longer depends on #27126.
| // The dynamic setting can change between constructing and publishing this cache. Re-read the volatile | ||
| // snapshot after insertion so that a cache missed by updateCopyEntriesInCache's concurrent iteration | ||
| // cannot remain on the old mode. | ||
| newEntryCache.setCopyEntries(copyEntriesInCache); |
There was a problem hiding this comment.
[P2] Serialize cache publication with copy-mode updates
The volatile read and setCopyEntries call are separate operations, so this can still leave the newly published cache on the old mode:
- The creator reads
copyEntriesInCache == falseas the setter argument and is descheduled. updateCopyEntriesInCache(true)stores the new mode and visits this already-published cache, setting it totrue.- The creator resumes and calls
setCopyEntries(false).
Both methods then return with the manager/configuration set to true but this cache set to false. Future inserts continue retaining receive buffers despite copying being enabled; the sequential tests do not cover this interleaving.
Please serialize publication/mode assignment with updates, or have inserts consult a single manager-owned volatile policy instead of maintaining independently updated per-cache copies.
There was a problem hiding this comment.
This race was fixed by using one shared volatile copy policy for cache inserts, with a regression for the publication/update interleaving. The implementation and tests have since been split out of this PR into #27148, which is now merged. See the updated follow-up reply.
codelipenghui
left a comment
There was a problem hiding this comment.
Follow-up validation of 346ec2490abbd78978c3d8646638cacff54cc6b0, updating the earlier source-only assessment of this head.
Focused validation now confirms the fixes for pooled-root accounting, in-place capacity growth, and stale throttle notifications. Thirty-five cases passed: 16 decoder cases, 14 controller cases, two broker integration tests, and three adapted local regression probes.
The same two outstanding findings remain:
- P1: queued-frame progress still fails on this exact head, now reproduced with
PooledByteBufAllocator.DEFAULT. The original-decoder control delivers both frames; the tracking decoder delivers only one after the other throttle clears. - P2: cache creation can still overwrite a newer dynamic copy-mode setting. This remains a source-established race; no runtime concurrency reproduction is claimed.
Requesting changes for these two issues. The queue regression uses EmbeddedChannel, the real decoding/flow-control/throttle components, and a mocked ServerCnx; it is not a production workload reproduction. No additional confirmed defects were identified in the reviewed delta.
| if (throttled) { | ||
| serverCnx.getThrottleTracker().markThrottled( | ||
| ServerCnxThrottleTracker.ThrottleType.InProgressRequestFrameMemoryExceeded); |
There was a problem hiding this comment.
[P1] Follow-up: queued-frame stall reproduced on this head with pooled buffers
I reran the combined-throttle regression against 346ec2490a using PooledByteBufAllocator.DEFAULT, the actual decoder, PulsarFlowControlHandler, and ServerCnxThrottleTracker with a mocked connection. After releasing the first frame and clearing TopicPublishRate, the tracking case still delivered only one of two frames; the original length-field-decoder control delivered both.
The circular dependency remains: the queued tail retains the charged root allocation, but this throttle disables auto-read, which also prevents the flow-control queue from draining. Deferring the notification does not help when another throttle has already queued that tail.
Please separate stopping additional socket intake from draining already-accounted frames, or otherwise ensure that the work retaining the allocation can complete. This needs a regression combining memory pressure with another throttle reason and pooled sibling frames.
There was a problem hiding this comment.
The fix is proposed separately in #27126, which removes PulsarFlowControlHandler from the broker pipeline. Throttling then pauses subsequent automatic socket reads while complete frames already read continue to ServerCnx. This eliminates the queued-tail dependency that keeps an allocation alive while waiting for the memory throttle to clear.
#27126 is still open, so this PR remains dependent on that change. After it is merged, it needs to be integrated here and the combined tracking-decoder/other-throttle regression rerun. This issue is not yet resolved in the current head of this PR.
There was a problem hiding this comment.
I agree that removing the flow-control queue can break the circular dependency here. However, #26013 restored per-message throttling for a real publish-rate regression, so #27126 changes an existing behavior that needs careful validation.
I'd keep this blocked until #27126 is integrated and the combined-throttle regression passes on the updated head. We should also verify the impact on publish-rate overshoot, pending publish requests, and memory usage under realistic workloads.
There was a problem hiding this comment.
Update for @codelipenghui and @Denovo1998: the fix is now included in this PR in bd7903c4e3. This supersedes my earlier reply about depending on #27126; this fix no longer requires removing the flow-control handler.
PulsarFlowControlHandler and the existing handler order are retained. Request-frame memory throttling pauses new socket intake, while already-decoded frames can still be delivered when no business throttle is active. Clearing the last Topic/pending-publish/other business throttle schedules a drain of the existing queue even while memory throttling keeps auto-read disabled. Delivery rechecks business throttles for every frame, and draining an empty queue does not request more socket data.
The regression tests include the reported pooled sibling-frame case with Topic throttling, both release orders for every business throttle, throttling again during a drain, incomplete-frame continuation, and connection closure. They use the real decoder/flow-control/tracker components with an EmbeddedChannel and a mocked ServerCnx.
This addresses the queued-tail circular dependency while preserving per-message business backpressure. The design is also documented in PIP-499. The fixed-load performance results in the PR description do not establish publish-rate overshoot or pending-publish behavior under production memory pressure; those results should not be read as that broader validation.
| // The dynamic setting can change between constructing and publishing this cache. Re-read the volatile | ||
| // snapshot after insertion so that a cache missed by updateCopyEntriesInCache's concurrent iteration | ||
| // cannot remain on the old mode. | ||
| newEntryCache.setCopyEntries(copyEntriesInCache); |
There was a problem hiding this comment.
[P2] Follow-up: cache copy-mode publication race remains unchanged
The creator can read false as this setter's argument and then be descheduled. Meanwhile, updateCopyEntriesInCache(true) stores the new policy and visits this already-published cache, setting it to true. When the creator resumes, setCopyEntries(false) overwrites that update. Both methods then return with configuration true but the new cache still retaining receive buffers on future inserts.
This read-then-set sequence is not atomic with the updater's iteration; making the fields volatile does not prevent the interleaving. The relevant code is unchanged in this revision. This finding is based on source analysis, not an executed race test.
Please serialize publication/mode assignment with updates, or make inserts consult one manager-owned volatile copy policy, and add concurrent creation/update coverage.
There was a problem hiding this comment.
Fixed in 74852f3, with the exact publication-race regression added in cb808b5.
Caches no longer keep separate copies of the policy. Single-entry and batch insertions use the shared volatile ManagedLedgerFactoryConfig.copyEntriesInCache through the manager, eliminating both the publication-time setter and per-cache update propagation. Existing cached entries are preserved, and BookKeeper batch reads still force copying.
The new regression pauses the creator at the legacy setter entry, after its argument has been evaluated but before assignment. It completes one concurrent configuration update, verifies that the cache uses the new mode, then resumes the creator and checks actual buffer reference counts again. Construction interception delegates operations to a real cache; no production test hook or private-state manipulation is needed. Both enable/disable cases fail against the old implementation and pass with the fix.
Validation: all 40 related tests passed with retries disabled, and full ./gradlew quickCheck passed, including main/test Checkstyle, Spotless, and RAT.
There was a problem hiding this comment.
Update: the shared copy-policy fix and its publication-race regression were split into #27148, which has now been merged.
Cache inserts consult the shared volatile policy instead of receiving separately propagated per-cache values, so the stale publication-time assignment described here is eliminated. The existing managedLedgerCacheCopyEntries setting remains independently configurable; updates affect future inserts without clearing existing cached entries.
The EntryCache implementation and tests were removed from this PR in 81730ae. They are reviewed and maintained in the separate, merged change.
Read a shared volatile copy policy on cache insertion instead of propagating per-cache snapshots that can overwrite dynamic updates. Preserve existing entries and forced copying of batch reads, and cover concurrent cache creation, overlapping updates, and asynchronous read completion. Assisted-by: Codex
Reproduce a captured stale setter argument with one concurrent policy update in each direction. Delegate inserts to a real cache and assert buffer reference counts before and after the creator resumes. Assisted-by: Codex
lhotari
left a comment
There was a problem hiding this comment.
Please file an issue to show exactly which problem this is addressing. In the issue, it would also be useful to share the reproducer of the issue.
This change might have a significant performance impact and before applying this change, there would need to be careful performance analysis. This could be tested with tests/performance scenarios on Linux x64_64/amd64 architecture and the actual Docker image since the malloc implementation also matters.
I think that this change needs a PIP since it's also labeled for 4.0.x and 4.2.x.
It also allows EntryCache to copy future entries so cached slices do not pin large inbound allocations.
This should be a separate change and also configurable.
…change Remove the managed-ledger copy policy updates and their tests from the request-frame memory tracking change. Keep the frame limiter lifecycle coverage independent of dynamic cache configuration. Assisted-by: Codex
|
@lhotari, following up on your request to split out the EntryCache change:
Moved the EntryCache dynamic configuration, shared copy-policy implementation, and related tests into #27148 ( Removed those changes from this PR in 81730ae and updated the description accordingly. This PR now contains only the request-frame memory tracking and limiting changes. |
Use validated optimistic state checks between transitions and coalesce connection notifications per event loop. Keep exact global accounting, generation thresholds, and release-driven recovery. Add concurrent recovery and notification lifecycle tests and a shared-controller JMH benchmark. Assisted-by: Codex
Sample direct memory outside the lock and validate the optimistic stamp before skipping unchanged automatic state. Include disabled, manual, and automatically enabled modes in the shared-controller benchmark. Assisted-by: Codex
Exercise 1,000 connection callbacks on a real Netty event loop and verify the last release from another thread resumes all of them without another inbound event. Always shut down the event loop on failure. Assisted-by: Codex
| InProgressRequestFrameMemoryLimiter inProgressRequestFrameMemoryLimiter = | ||
| pulsar.getBrokerService().getInProgressRequestFrameMemoryLimiter(); | ||
| ch.pipeline().addLast(FrameDecoderUtil.FRAME_DECODER_HANDLER, | ||
| new InProgressRequestFrameTrackingDecoder(brokerConf.getMaxMessageSize(), | ||
| inProgressRequestFrameMemoryLimiter)); |
There was a problem hiding this comment.
don't add this handler unless inProgressRequestFrameMemoryLimitEnabled=true. This avoids regressions for users that don't need this.
There was a problem hiding this comment.
Keeping the tracking decoder installed is intentional: continuously measuring retained request-frame memory is one of the goals of PIP-499. See Accounting for request-frame memory and Behavior when forced off.
DISABLED turns off request-frame throttling and automatic activation, but preserves accounting and the metric so operators can observe retention amplification while the limiter is off. Replacing the tracking decoder with the ordinary decoder in that mode would remove the information that this metric is intended to expose. The disabled path skips policy checks and direct-memory-utilization sampling; it still pays the accounting cost.
That cost is now reported in the PR description: in the native Linux amd64 / actual Docker-image fixed-load comparison, DISABLED used 2.35% more broker CPU per million messages than the no-PR baseline (three runs per variant). Keeping metrics available has a measured cost; the always-on accounting requirement is documented in the PIP for design review.
There was a problem hiding this comment.
There's a very high risk that this introduces regressions. I don't see a reason why this feature couldn't be completely disabled. I'm strongly against adding such a feature that is on the hot path of Pulsar and it includes a lot of synchronization.
There was a problem hiding this comment.
Inbound publish requests are decoded from Netty pooled direct buffers.
This is very different with master/branch-5.0. The default is AdaptiveByteBufAllocator. With adaptive allocator, less memory will be wasted compared to PooledByteBufAllocator.
Another important detail is that using an Alpine base image results in higher memory consumption (RSS) due to the poor malloc implementation. In Pulsar 5.x+, mimalloc is used to address this. A similar approach could be added to 4.x.
However, the biggest impact for the receive buffers happen with AdaptiveRecvByteBufAllocator parameters which are hardcoded in Pulsar:
With sustained traffic, the receive buffer size will grow up to 1 MB. Adjusting that would be necessary to actually make an impact. Without changing that, all connections pushing a lot of data will eventually have receive buffers of 1 MB.
PIP-430 suggested making AdaptiveRecvByteBufAllocator parameters configurable, but that hasn't been implemented:
Lines 196 to 224 in 73c75fe
I've previously requested this:
Please file an issue to show exactly which problem this is addressing. In the issue, it would also be useful to share the reproducer of the issue.
Unless there's a reproducer, it's also hard to evaluate whether this PR is really solving the actual problem.
…uests Add DISABLED, AUTO and ENABLED request-frame limiting modes, and apply the latest dynamic memory threshold to all connections. Separate socket backpressure from queued frame delivery so memory throttling permits buffered requests and incomplete frames to make progress. Cover threshold updates, automatic-mode transitions, concurrent releases, pooled sibling frames, overlapping business limits and connection cleanup. Verified targeted broker tests, microbenchmark compilation and quickCheck. Assisted-by: Codex
Add a broker-wide OpenTelemetry gauge for the active throttle state and cover dynamic configuration and half-limit recovery in metric export tests. Assisted-by: Codex
|
@lhotari, following up on the PIP, reproducer, allocator and performance requests in this review and the follow-up:
|
| /** Track retained memory without applying request-frame backpressure, even under memory pressure. */ | ||
| DISABLED, |
There was a problem hiding this comment.
There should be a mode to completely disable this feature. This doesn't seem to be the case. It's misleading to call this DISABLED.
There was a problem hiding this comment.
renamed DISABLED to TRACKING, and added new DISABLED
Motivation
Inbound publish requests are decoded from Netty pooled direct buffers. A small retained frame can pin a much larger cumulation allocation, while the existing publish-buffer limiter accounts for logical payload bytes rather than retained root-buffer capacity. This can leave substantial direct memory retained by in-progress requests before the broker applies backpressure.
Modifications
This change tracks the capacity of direct-memory root buffers retained by inbound request frames, including the extra capacity pinned by small slices. It adds configurable broker-wide backpressure with three modes: metrics only (
DISABLED), automatic memory-pressure protection (AUTO, the default), and forced enforcement (ENABLED). Separate retained-memory and active-throttling metrics make both memory use and the limiter's current state observable.InProgressRequestFrameTrackingDecoder, preserving accounting across slices, duplicates, cumulation replacement/expansion, and final release on any thread.maxMessagePublishBufferSizeInMBre-evaluate existing and future connections against the latest broker-wide threshold; the existing payload-byte limiter keeps its original connection-time configuration behavior.DISABLEDmaintains accounting but skips policy evaluation;AUTOchecks direct-memory pressure only when activation or recovery is possible;ENABLEDapplies the retained-capacity threshold regardless of direct-memory pressure. Boolean aliases remain accepted:falsemaps toAUTO, andtruetoENABLED.PulsarFlowControlHandler. Request-frame memory throttling stops new socket reads while allowing queued frames to complete and release their roots; other throttle reasons continue to pause queue delivery. Recheck those reasons after each frame and permit bounded continuation reads for an incomplete frame when memory throttling is the only reason for the pause.pulsar.broker.publish.in.progress.buffer.memory.usage(retained root capacity in bytes) andpulsar.broker.publish.in.progress.buffer.memory.throttled(1 while this limiter is throttling, otherwise 0). The latter describes active request-frame throttling, not whether its policy is enabled and not other broker throttling reasons.managedLedgerCacheCopyEntriesimplementation and its managed-ledger tests in the independent PR [improve][broker] Make entry cache copying dynamically configurable #27148.Performance comparison: no PR vs. the complete PR
The comparison uses
17166c820bf011ce7dd8b2b6795cc9c62e734af7(without this PR, its common ancestor) andef2a4475722d94c0d673a317f9c0d82993c56251(the complete PR, including the three modes, shared dynamic threshold, flow-control fix, and throttling metric). The harness, clients and Dockerfile are identical across these revisions. Both images were built before measurement, and each report records its exact commit and a clean checkout. These numbers replace the previous performance table.All runs used one native Linux x86_64 GitHub-hosted VM: AMD EPYC 7763, 4 vCPUs, approximately 16 GiB RAM, Ubuntu 24.04.5 and Docker 28.0.4. Images used Alpine 3.24.3, Amazon Corretto 25.0.5 and the Pulsar Dockerfile's mimalloc/gcompat preload. Both sides enabled the OpenTelemetry Prometheus exporter with the same 1-second scrapes. No emulation or profiling was used. The earlier experiment used a different host and telemetry configuration; its numbers are not used as this experiment's baseline.
The IoT workload used 10,000 devices, 10 gateway clients, 8 topics, and one application with one Key_Shared consumer client. One broker, one bookie (E/W/A=1), and ZooKeeper shared the host. Broker and bookie each had 512 MiB heap and 512 MiB direct-memory limits. Batching was disabled; broker IO threads=4;
maxMessagePublishBufferSizeInMB=256;managedLedgerCacheCopyEntries=false. The bookie journal used the framework's 2 GiB tmpfs overlay with journal sync disabled. This is a test topology, not a production durability configuration.There are 12 completed small-message runs: 128 B payloads at 20,000 msg/s, with a global outstanding-send window of 1,024, 20 seconds of warmup and 60 seconds of measurement. Each variant has three runs, ordered no-PR/DISABLED/AUTO/ENABLED; ENABLED/AUTO/DISABLED/no-PR; AUTO/no-PR/ENABLED/DISABLED. Resolved configurations match apart from the PR-only mode and generated service URL.
The experiment also completed seven 128 KiB runs at 500 msg/s (window=128; same warmup and measurement duration). Their repetition counts are unequal, so they are reported separately as exploratory observations. Two short metric-collection checks are excluded from performance results. The next large-message baseline run was interrupted by cancellation and has no complete measurement report; it is excluded. Neither unpaced scenario was started.
128 B: three runs per variant
Values are median [minimum–maximum] across runs. Publish p99 is the median of per-run p99s, not a pooled percentile. Broker CPU comes from cgroup measurements normalized per million measured messages. Throughput is higher-is-better; latency and CPU per message are lower-is-better.
The manual-enable and throttle columns refer only to request-frame memory limiting; the legacy payload-byte limiter remains enabled throughout. The limit column is
maxMessagePublishBufferSizeInMB. Throttle observations count samples equal to 1 over samples within the publish measurement windows. Zero samples mean not observed, and do not rule out a throttle shorter than the sampling interval.Delivery p99 medians [min–max] were 77.1 [74.0–98.0] ms without the PR; 75.0 [74.0–96.1] ms in DISABLED; 91.0 [88.1–123.0] ms in AUTO; and 109.1 [90.0–109.1] ms in ENABLED. Maximum observed retained-frame capacities across the three runs were 39.58, 37.80 and 39.14 MiB respectively; these are sampled values, not exact peaks.
128 KiB: incomplete repetition set, exploratory only
The baseline has only one completed run and each PR mode has two. The ranges below are individual-run observations, not a statistically supported comparison or an estimated PR speedup/regression. All seven runs sustained approximately 500 msg/s.
Maximum sampled retained-frame capacities were 162.25 MiB in DISABLED, 159.50 MiB in AUTO and 162.50 MiB in ENABLED.
What the results establish
always; the governor wasperformance, with no exposed turbo control. CPU frequency and thermal-throttle samples were unavailable. Host tuning was not changed. Without profiles, these results cannot isolate accounting, locks, allocations, GC or scheduler contention as the cause of an observed change.Reproduction and evidence
The comparison workflow was intentionally stopped after the runs above; its overall status is cancelled. The pinned driver and workflow are on a separate experimental branch, outside this PR. The complete small-message comparison can be reproduced with the driver phases
prepare,verify-metrics, andfixed-smallon one Linux amd64 host.The saved raw-results artifact,
request-frame-modes-comparison, is currently retained until 2026-11-27. It contains the manifest, reports, resolved configurations, exact commits/commands, host and image metadata, latency histograms, cgroup CPU summaries, memory metric JSON, and timestamped retained-frame/throttle samples. Only manifest entries withmeasurementRun=trueandvalid=trueare included above. Use each entry'sreportpath and the producer measurement timestamps ingateways-summary.jsonto reproduce the filtering.Does this pull request potentially affect one of the following parts:
Best practice
Use
inProgressRequestFrameMemoryLimitEnabled=AUTO(the default) for broker-local memory-pressure protection. It activates when direct-memory usage reaches at least 85% and retained frame capacity exceeds 1.5 ×maxMessagePublishBufferSizeInMB; enforcement starts on the next inbound read. Automatic protection turns off when direct-memory usage falls below 65%. UseENABLEDto enforce the retained-frame threshold regardless of direct-memory pressure, orDISABLEDfor metrics-only operation with no automatic activation.When this policy is active, all connections share the latest dynamically configured
maxMessagePublishBufferSizeInMB: retained frame capacity above the limit pauses socket reads, and capacity at or below half the limit clears that throttle. A nonpositive value disables this limiter. Dynamic mode and numeric-limit changes affect existing connections; final buffer releases drive recovery without a periodic polling task. Other broker throttle reasons remain independent.Monitor both
pulsar.broker.publish.in.progress.buffer.memory.usageandpulsar.broker.publish.in.progress.buffer.memory.throttledthrough the configured OpenTelemetry exporter. The first reports retained root-buffer capacity, not allocator-reserved memory or RSS; the second reports current request-frame throttling (0/1), not policy enablement. EnablemanagedLedgerCacheCopyEntriesindependently when EntryCache may retain inbound slices; support for dynamically changing that existing setting is provided by #27148.