[fix][client] Apply no-memory-limit producer queue defaults at producer creation - #26342
[fix][client] Apply no-memory-limit producer queue defaults at producer creation#26342lhotari wants to merge 3 commits into
Conversation
…er creation ### Motivation The client memory limit is a producer's primary backpressure: it bounds the memory held by messages that have been queued but not yet acknowledged by the broker. apache#15723 added a safety net for clients that disable it, so that producers fall back to a bounded pending-message queue instead of buffering without any limit: ```java public <T> ProducerBuilder<T> newProducer(Schema<T> schema) { ProducerBuilderImpl<T> producerBuilder = new ProducerBuilderImpl<>(this, schema); if (!memoryLimitController.isMemoryLimited()) { producerBuilder.maxPendingMessages(NO_MEMORY_LIMIT_DEFAULT_MAX_PENDING_MESSAGES); producerBuilder.maxPendingMessagesAcrossPartitions( NO_MEMORY_LIMIT_DEFAULT_MAX_PENDING_MESSAGES_ACROSS_PARTITIONS); } return producerBuilder; } ``` That net has two holes, and either one leaves a producer with no bound at all, because `ProducerImpl` only creates its semaphore `if (conf.getMaxPendingMessages() > 0)`: 1. It is only applied by the `newProducer(Schema)` overload. The no-argument `newProducer()` returns a plain builder, so it never gets the fallback. In-tree users of that overload include the Functions log appender and the WebSocket proxy's producer handler. 2. It is applied when the builder is constructed, so a later `maxPendingMessages(0)` overwrites it. Since `ProducerConfigurationData.DEFAULT_MAX_PENDING_MESSAGES` is 0, code that passes the default through explicitly silently disables the fallback rather than keeping it. ### Modifications - Resolve the pending-message limits at producer creation, in `PulsarClientImpl`'s `createProducerAsync(conf, schema, interceptors)`, instead of on the builder. That is the funnel every producer built from this client passes through, so the fallback no longer depends on which `newProducer` overload created the builder, and cannot be undone by a later call setting a limit back to its unset value. Only unset limits are filled in; an explicit limit is never overwritten. - Cap the per-producer fallback by the across-partitions limit. That limit is a budget shared by every partition, and its setter rejects a value below `maxPendingMessages`, so filling in the larger default first would throw `IllegalArgumentException` synchronously out of a method that returns a `CompletableFuture`. - Resolve on a copy of the configuration, so filling in a limit does not leak into the next producer built from the same builder. - Remove the now-redundant block from `newProducer(Schema)`. This also fixes a side effect it had: on a client with the memory limit disabled, `newProducer(schema).maxPendingMessagesAcrossPartitions(500)` used to throw, because the builder had already been given a `maxPendingMessages` of 1000. - Document on `ProducerBuilder` what disabling either check actually means. The V5 client reaches producer creation through `createSegmentProducerAsync` and is deliberately left unchanged here; it exposes no pending-message setting of its own and needs a separate decision. Note that `pulsar-perf` on master uses the V5 client, so this change on its own does not alter its behaviour. ### Verifying this change Added tests, each confirmed to fail before the fix: - `ProducerQueueSizeTest#testNoArgNewProducerIsBoundedWhenMemoryLimitDisabled` (hole 1) - `ProducerQueueSizeTest#testLateZeroMaxPendingMessagesDoesNotDisableTheBoundWhenMemoryLimitDisabled` (hole 2) - `ProducerQueueSizeTest#testPartitionedProducerIsBoundedWhenMemoryLimitDisabled` - `ProducerQueueSizeTest#testExplicitMaxPendingMessagesAboveTheFallbackDoesNotFailCreation` and `#testExplicitAcrossPartitionsLimitCapsTheFallback`, which pin that filling in a default never fails producer creation - `ProducerQueueSizeTest#testFallbackLeavesTheBuilderReusable`, which pins that the resolved configuration is a copy `ProducerQueueSizeTest#testMemoryLimitedClientKeepsUnboundedPendingMessages` pins that a client with a memory limit configured is unaffected. Noticed while working on this, left alone as a separate concern: when `maxPendingMessagesAcrossPartitions` is explicitly set below the topic's partition count, the per-partition share in `PartitionedProducerImpl` rounds down to 0, which means "no limit". So a tighter budget produces a looser bound, and an explicitly configured `maxPendingMessages` is silently discarded. On a client with the memory limit disabled it can also defeat the fallback applied here, since the filled-in limit is divided by that same code: an explicit budget of 500 on a topic with 501 partitions still ends up unbounded. It affects clients regardless of their memory limit, and clamping the share turned out to change behaviour for memory-limited clients too, so it needs its own change rather than riding along here. ### Does this pull request potentially affect one of the following parts: - [x] The default values of configurations A producer created on a client whose memory limit is disabled now has a bounded pending-message queue where it previously had none. This is the behaviour apache#15723 intended; only the cases where it did not take effect change. Specifically: - Because `maxPendingMessages` is a primitive `int` whose unset value is 0, an application that explicitly passed 0 to mean "unbounded" cannot be distinguished from one that never set it, and now gets the bound as well. Such an application can keep an unbounded message count by configuring a client memory limit, which bounds the queue by bytes instead, or by setting an explicit `maxPendingMessages`. - On a partitioned topic, an application that set `maxPendingMessages` but left `maxPendingMessagesAcrossPartitions` unset now has the filled-in budget divided between the partitions, which can lower its per-partition limit. This matches what the `newProducer(Schema)` overload already did. - The WebSocket proxy is affected out of the box, since `webSocketPulsarClientMemoryLimitInMB` defaults to 0 and its producers use the no-argument `newProducer()` with `blockIfQueueFull` false. A client with more than 1000 unacknowledged messages in flight now gets a failed `ProducerAck` instead of the proxy buffering them. Raising the limit through the `maxPendingMessages` query parameter currently also requires enabling batching, which is worth fixing separately.
|
Now, the producer has backpressure, how to disable backpressure? |
good question. I'll check and revisit that. The |
…an maxPendingMessages
### Motivation
`ProducerConfigurationData.setMaxPendingMessagesAcrossPartitions` rejected any value below
`maxPendingMessages`. That makes the two setters order-dependent, and it makes
`ProducerBuilder.loadConf` fail outright for any positive `maxPendingMessages`:
`ConfigurationDataUtils.loadData` serialises the configuration, merges the caller's map and
deserialises a new instance by replaying every property through the public setters, in an order
the caller does not control. So
builder.loadConf(Map.of("maxPendingMessages", 5000))
throws `maxPendingMessagesAcrossPartitions needs to be >= maxPendingMessages`, because the
across-partitions property that comes along with the merged map is still at its default of 0.
The same check also makes a builder reject a legitimate call sequence: setting a per-producer
limit and then a smaller shared budget throws, while the reverse order is accepted.
### Modifications
Validate only that the value is not negative. The relationship between the two limits is enforced
where it is used: `PartitionedProducerImpl` lowers the per-partition limit to its share of the
budget whenever a budget is set, and the budget is meaningless on a non-partitioned topic.
Assisted-by: Claude Code (Opus 5)
…where unset ### Motivation When the client memory limit is disabled there is no byte-based backpressure left, so apache#15723 gave producers the pre-PIP-120 pending-message defaults instead of letting them buffer without any limit. Its commit message states the intent: "restore maxPendingMessages and maxPendingMessagesAcrossPartitions when memory limit is disabled so that pre-PIP-120 default configuration is restored when limit is disabled". They are defaults, and PIP-120 is the same commit that changed them to 0 and documented 0 as "disable the pending messages check". An application that configures a limit therefore has to keep winning over them, including with an explicit 0. The previous approach seeded the defaults onto the builder in `newProducer(Schema)`, which left three holes: 1. The no-argument `newProducer()` never got them, so the WebSocket proxy's producer handler, the Functions log appender and the Functions worker are unbounded today. 2. A caller that later passed a limit through - a CLI flag or a config value sitting at its own default - silently replaced them. 3. Seeding `maxPendingMessages` to 1000 made a later `maxPendingMessagesAcrossPartitions(500)` throw, which a Functions `ProducerConfig` setting only that limit walks straight into. ### Modifications `maxPendingMessages` is a primitive whose unset value is 0, and 0 is also a meaningful explicit value, so the configuration alone cannot tell "never configured" from "explicitly unbounded". `ProducerBuilderImpl` records which of the two limits the application configured - through the setters or through `loadConf` - and carries that across `clone()`. At producer creation the client fills in only the limits that were never configured, on a copy of the configuration so nothing leaks into the next producer built from the same builder. A limit that is already positive counts as configured however the configuration was populated. Tracking this on the builder rather than on the configuration is not a preference: `loadConf` goes through `ConfigurationDataUtils.loadData`, which rebuilds the configuration by replaying every property through the setters, so a marker held there would be re-set on every call. An explicit `maxPendingMessages(0)` also suppresses the across-partitions default, so one call is enough to ask for a producer with no message-count limit whatever the topic's shape - filling in the budget would put a per-partition limit straight back. The V5 client is deliberately left out. It reaches producer creation through `createSegmentProducerAsync` and exposes no pending-message setting at all, so its client memory limit is the only backpressure it has, and the only thing an application can turn off. The WebSocket proxy takes `maxPendingMessages` from a query parameter and its client has no memory limit by default, so a remote client could now ask for an unbounded pending queue inside the shared proxy. A non-positive value is ignored there. Assisted-by: Claude Code (Opus 5)
You were right — I've reworked the PR so that it can be disabled again. Why the first revision got this wrong. #15723 introduced the 1000/50000 values to "restore ... pre-PIP-120 default configuration ... when [the memory] limit is disabled" (563a7cb), and PIP-120 (11bfc0e) is the commit that both changed those defaults to 0 and documented 0 as "disable the pending messages check". So they are defaults, and an explicit 0 has to keep meaning "no message-count limit". The first revision turned them into a floor, because it inferred "unset" from What it does now.
So, to disable backpressure entirely — one call for each of the two mechanisms: PulsarClient client = PulsarClient.builder()
.serviceUrl(url)
.memoryLimit(0, SizeUnit.BYTES) // no byte-based backpressure
.build();
Producer<byte[]> producer = client.newProducer()
.topic(topic)
.maxPendingMessages(0) // no message-count backpressure
.create();An explicit Why the flag is on the builder and not on the configuration. I tried the configuration first. One bug found on the way, in its own commit. The CLI tools. I thought this PR would have to cover them, but on master they are all on the V5 client now: V5 is deliberately untouched, and the reason is now in the code: it exposes no pending-message setting at all, so the client memory limit is both its only backpressure and the only thing an application can turn off. Filling in a message-count default there would leave a V5 user with no way to say "unbounded" at all. One thing added beyond the client: the WebSocket proxy takes I've updated the PR description with the full behaviour table and the compatibility notes. Since the mechanism changed, could you take another look? |
|
@lhotari LGTM, thanks |
Motivation
The client memory limit is a producer's primary backpressure: it bounds the memory held by messages that have been queued but not yet acknowledged by the broker. #15723 added a safety net for clients that disable it, so that producers fall back to a bounded pending-message queue instead of buffering without any limit. Its commit message states the intent exactly:
PIP-120 (#13344) is what changed those two defaults from 1000/50000 to 0 and made the client memory limit the primary mechanism. So 1000/50000 are defaults — a value the application did not configure — and an application that configures a limit has to keep winning over them, including with an explicit
0, which PIP-120 documented as "disable the pending messages check".That net was applied on the builder:
and it has three holes, the first two of which leave a producer with no bound at all, because
ProducerImplonly creates its semaphoreif (conf.getMaxPendingMessages() > 0):newProducer(Schema)overload. The no-argumentnewProducer()returns a plain builder, so it never gets the defaults. In-tree users of that overload are the WebSocket proxy's producer handler, the Functions log appender and the Functions worker — all on clients whose memory limit is disabled, so all unbounded today.maxPendingMessages(0)is the point of this PR, see below.maxPendingMessagesto 1000 makes a latermaxPendingMessagesAcrossPartitions(500)throwIllegalArgumentException, because that setter requires its value to be>= maxPendingMessages. This is reachable in production:ProducerBuilderFactorybuilds fromnewProducer(schema)on a Functions client that is hard-wired tomemoryLimit(0), and setsmaxPendingMessagesAcrossPartitionsindependently ofmaxPendingMessages. A function configured with onlymaxPendingMessagesAcrossPartitions: 500fails to create its producer.This came up while investigating #26340, where
pulsar-perfexhausts direct memory against a slow broker. On branch-4.0 its producer hits holes 1 and 2 at once: it calls the no-argumentnewProducer()and then passesmaxPendingMessages(0)through from its own default. #26341 fixes that tool's--memory-limitdefault; this PR closes the client-side holes that let the safety net be bypassed in the first place.This PR does not close #26340 on its own.
pulsar-perfon the maintenance branches still passes its own unset--max-outstanding/--max-outstanding-across-partitionsdefaults straight to the builder, which under these semantics reads as an explicit "no limit", and on a non-partitioned topic nothing else bounds it. That needs the same treatment #15283 gavemaxPendingMessagesAcrossPartitions— an unset value the tool can recognise, rather than a0that means two things — and it is apulsar-testclientchange on branch-4.2/branch-4.0 only: #25887 removed those options' effect on master whenpulsar-perfmoved to the V5 client. Filed separately.Modifications
The unset value of
maxPendingMessagesis0, and0is also the documented value for "no message-count limit". So "never configured" and "explicitly unbounded" cannot be told apart from the configuration alone, and the fix is to record which of the two it is:ProducerBuilderImplremembers whether the application calledmaxPendingMessages(...)/maxPendingMessagesAcrossPartitions(...), or passed either of them toloadConf. The state is carried throughclone().newProducer(Schema)no longer seeds the builder. Resolving at creation instead means the defaults no longer depend on whichnewProduceroverload produced the builder (hole 1), cannot be replaced by a pass-through (hole 2), and cannot make a later setter call throw (hole 3).The result is that the two limits behave as ordinary defaults. On a client whose memory limit is disabled:
maxPendingMessagesmaxPendingMessagesAcrossPartitionsmaxPendingMessages(500)maxPendingMessagesAcrossPartitions(500)maxPendingMessagesAcrossPartitions(0)maxPendingMessages(0)The last row is how an application asks for a producer with no backpressure at all, which is what the previous revision of this PR took away. An explicit
maxPendingMessages(0)also leaves the across-partitions budget alone, so that one call is enough whatever the topic's shape: filling the budget in would put a per-partition limit back, becausePartitionedProducerImplderives it from that budget.Separately, and in its own commit because it is an independent defect:
setMaxPendingMessagesAcrossPartitionsrejected a value belowmaxPendingMessages, which made the two setters order-dependent and madeProducerBuilder.loadConffail outright for any positivemaxPendingMessages—ConfigurationDataUtils.loadDatareplays every property through the setters in an order the caller does not control, soloadConf(Map.of("maxPendingMessages", 5000))throwsmaxPendingMessagesAcrossPartitions needs to be >= maxPendingMessageson master today. The check is now>= 0; the relationship is enforced where it is used, inPartitionedProducerImpl, which already lowers the per-partition limit to its share of the budget. This is whatProducerBuilderImplTest#testLoadConfWithAPositiveMaxPendingMessagespins, and it is whyloadConfcan be treated as a way to configure these limits at all. Happy to split it out if you would rather review it separately.Two things this deliberately does not do:
createSegmentProducerAsyncand exposes no pending-message setting at all, so its client memory limit is the only backpressure it has — and therefore the only thing an application can turn off. Filling in a message-count default there would leave a V5 user with no way to express "unbounded". Note thatpulsar-perfon master uses the V5 client, so this change on its own does not alter its behaviour there.ProducerConfigurationData.ProducerBuilder.loadConfgoes throughConfigurationDataUtils.loadData, which serialises the configuration to JSON and deserialises a new object through the public setters. A primitive is always emitted, sosetMaxPendingMessages(0)always runs on the way back in: a@JsonIgnoremarker on the configuration would be set on everyloadConfcall, and a serialised one would be applied inHashMapkey order. The builder is the only place that sees what the application actually called.Outside the client:
maxPendingMessagesfrom a query parameter, and its client runs without a memory limit by default (webSocketPulsarClientMemoryLimitInMBis 0). Now that an explicit 0 is honoured,?batchingEnabled=true&maxPendingMessages=0would let a remote client ask for an unbounded pending queue inside the shared proxy, withblockIfQueueFullfalse. A non-positive value is now ignored so the client's default applies.ProducerBuilder's javadoc documents what the defaults are and how to opt out.Noticed but deliberately left alone
AbstractReplicatorpasses the broker'sreplicationProducerQueueSizestraight tomaxPendingMessages, so setting that config to 0 leaves a replicator producer unbounded — the replication client has no memory limit either. That is the behaviour on master today and this PR does not change it; a value of 0 is much more likely a misconfiguration than a request for an unbounded queue, but the config is operator-set and undocumented for 0, so tightening it belongs in its own change.PartitionedProducerImplwrites the per-partition share back into the configuration object it was handed (conf.setMaxPendingMessages(...)). When no default is filled in, that object is the builder's own, so creating a partitioned producer lowers the limit of the next producer built from the same builder. That is pre-existing and unrelated to the memory limit; the fallback path is not affected here because it resolves on a copy.maxPendingMessagesAcrossPartitionsis explicitly set below the topic's partition count, the per-partition share inPartitionedProducerImplrounds down to 0, which means "no limit". So a tighter budget produces a looser bound, and an explicitly configuredmaxPendingMessagesis silently discarded. I tried clamping that share to at least 1 and reverted it: the branch it lives in is entered whenever the value is set, with no memory-limit condition, so the clamp also changes behaviour for memory-limited clients — the default configuration. There, an across-partitions budget smaller than the partition count currently yields no semaphore and a working, bytes-bounded producer; clamping turns it into a single permit, and with the defaultblockIfQueueFull=falsethe second concurrent in-flight message per partition fails withProducerQueueIsFullError. That is a much larger blast radius than this PR should carry.Verifying this change
This change added tests and can be verified as follows.
Pinning the holes (each was confirmed to fail before the fix):
ProducerQueueSizeTest#testNoArgNewProducerIsBoundedWhenMemoryLimitDisabled— hole 1ProducerQueueSizeTest#testPartitionedProducerIsBoundedWhenMemoryLimitDisabledProducerQueueSizeTest#testFallbackLeavesTheBuilderReusable— pins that the resolved configuration is a copy, and that a latermaxPendingMessagesAcrossPartitionscall is validated against what the caller configured rather than against a filled-in default (hole 3)ProducerQueueSizeTest#testExplicitMaxPendingMessagesAboveTheFallbackDoesNotFailCreationand#testExplicitAcrossPartitionsLimitCapsTheFallback— pin that filling in a default never fails producer creationPinning that the defaults stay defaults. Each of these fails if "unset" is inferred from the value rather than from what the application configured — verified by reverting that one condition, which fails exactly these:
ProducerQueueSizeTest#testExplicitZeroDisablesTheBoundWhenMemoryLimitDisabled(partitioned and non-partitioned)ProducerQueueSizeTest#testExplicitZeroAcrossPartitionsKeepsThePerProducerDefaultProducerQueueSizeTest#testLoadConfZeroDisablesTheBoundWhenMemoryLimitDisabledProducerQueueSizeTest#testCloneKeepsAnExplicitlyDisabledBoundPlus
ProducerQueueSizeTest#testLoadConfWithoutTheLimitsKeepsTheDefaults(rebuilding the configuration is not mistaken for configuring it),#testMemoryLimitedClientKeepsUnboundedPendingMessages(a client with a memory limit is unaffected) andAbstractWebSocketHandlerTest#producerBuilderTest(the proxy ignores a non-positive query parameter).For the setter fix:
ProducerBuilderImplTest#testLoadConfWithAPositiveMaxPendingMessages(fails on master) and#testAcrossPartitionsLimitBelowMaxPendingMessagesIsAccepted.Also run locally: the whole
org.apache.pulsar.client.implsuite,pulsar-functions-instance,pulsar-websocket,ProducerSemaphoreTest,ProducerMemoryLimitTest,MemoryLimitTest,ConsumerMemoryLimitTest, and./gradlew quickCheck.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
A producer created on a client whose memory limit is disabled, and which does not configure a pending-message limit, now has a bounded pending-message queue where it previously had none. This is the behaviour #15723 intended; only the cases where it did not take effect change:
webSocketPulsarClientMemoryLimitInMBdefaults to 0 and its producers use the no-argumentnewProducer()withblockIfQueueFullfalse. A client with more than 1000 unacknowledged messages in flight now gets a failedProducerAckinstead of the proxy buffering them. Raising the limit through themaxPendingMessagesquery parameter currently also requires enabling batching, which is worth fixing separately.maxPendingMessagesbut leftmaxPendingMessagesAcrossPartitionsunset now has the filled-in budget divided between the partitions, which can lower its per-partition limit. This matches what thenewProducer(Schema)overload already did.maxPendingMessages=0no longer gets an unbounded producer.maxPendingMessagesAcrossPartitionsno longer throwsIllegalArgumentExceptionwhen it is set belowmaxPendingMessages; it is accepted, and the per-partition limit is lowered to its share as before. Only code that relied on the exception is affected, and such a call could not previously succeed.An application that passed
maxPendingMessages(0)explicitly to mean "unbounded" keeps that behaviour.Documentation
doc-requireddoc-not-neededdocdoc-completeThe behaviour is documented in the
ProducerBuilderjavadoc.