Repository navigation
[fix][cli] pulsar-perf: register subcommand classes with picocli instead of instances - #26467
Merged
Merged
Conversation
…ead of instances PulsarPerfTestTool.initCommander reflectively instantiated every registered subcommand before parsing a single argument, which ran each command class's static initializers on every invocation. `pulsar-perf produce` therefore allocated the latency histograms of consume, read, transaction and managed-ledger as well. picocli builds a subcommand's spec from annotations and defers creating the user object until that subcommand is selected, so registering the Class is enough. setDefaultValueProvider and setCaseInsensitiveEnumValuesAllowed propagate to the subcommand hierarchy as it exists when they are called, so both now run after the subcommands are registered; the latter used to be set by CmdBase on the per-command CommandLine it built in its own constructor, which is no longer the instance picocli parses with. This restores the behaviour that existed before apache#22388 (PIP-343), when bin/pulsar-perf exec-ed one main class per subcommand. Assisted-by: Claude Opus 5 (Claude Code)
lhotari
requested review from
Technoboy-,
dao-jun,
david-streamlio,
merlimat and
nodece
September 4, 2026 20:46
1 of 11 tasks
…ds load lazily Registering the command classes was not enough for ManagedLedgerWriter, CmdGenerateDocumentation and PerformanceClient: picocli has to create the user object in order to inject a @SPEC CommandSpec field, so those three were still initialized on every invocation while the five without @SPEC had become lazy. All three only used the injected spec to reach spec.commandLine(), and before this change that returned exactly the CommandLine CmdBase builds in its own constructor, because PulsarPerfTestTool registered ((CmdBase) o).getCommander() as the subcommand. Calling getCommander() directly is therefore the same object the code was already using, and it removes the last reason for picocli to instantiate these commands early. With this, `pulsar-perf produce` initializes PerformanceProducer and no other command class. Assisted-by: Claude Opus 5 (Claude Code)
merlimat
approved these changes
Sep 4, 2026
lhotari
added a commit
to lhotari/pulsar
that referenced
this pull request
Sep 7, 2026
…5 ones ### Motivation `pulsar-perf` was migrated to the V5 client API in apache#25887 (and apache#25917 did the same for `pulsar-client`), so every perf subcommand now drives the V5 SDK. That leaves no way to benchmark the v4 client, or v4 (non-scalable) topics, without the V5 SDK in the path — and several v4 capabilities became unreachable, because they have no V5 equivalent: - `--max-outstanding` / `--max-outstanding-across-partitions` and round-robin partition routing on the producer; - real `Exclusive` / `Failover` / `Key_Shared` subscription types, `MessageListener` dispatch on the client's listener threads, pooled messages, batch-index acknowledgment, the chunked-message knobs, the receiver-queue limits and the auto-scaled receiver-queue reporting on the consumer; - the v4 `Reader` itself — `read` measures a V5 `CheckpointConsumer`, a different broker-side entity — along with a `lid:eid` start message id, `--receiver-queue-size` and `--use-tls`; - the v4 transaction coordinator, which stays a live broker code path next to the v5 one (PIP-473 P5.4) with nothing driving it, and its acknowledgement round-trip latency (V5's `acknowledge` is a synchronous void, so its reported ack latency is a local measurement). ### Modifications Each benchmark is split into an abstract base holding everything that is not client-specific — the CLI options, the throughput/latency accounting, the run() skeleton and message loop, and the reports — plus two thin subclasses that bind the client types: the existing V5 command, and a new v4 one. PerformanceProducerBase -> PerformanceProducer / PerformanceProducerV4 (produce-v4) PerformanceConsumerBase -> PerformanceConsumer / PerformanceConsumerV4 (consume-v4) PerformanceReaderBase -> PerformanceReader / PerformanceReaderV4 (read-v4) PerformanceTransactionBase -> PerformanceTransaction / PerformanceTransactionV4 (transaction-v4) This is a split rather than a revert, so the bases keep the improvements made since the migration — instance recorders, the 3-significant-digit histograms and the latency clamps from apache#26466, class-based subcommand registration from apache#26467 — and both commands share one benchmark and one set of measurements. The v4 commands offer the same flags as their V5 counterparts (only the V5-only `--scalable-consumer-type` and `--scalable` are not mirrored). Each base logs through a logger named after the concrete subclass, so the report lines keep naming the subcommand that produced them. Where the two clients genuinely differ, the seam is explicit: the v4 consumer and reader keep `MessageListener`/`ReaderListener` dispatch while V5 keeps its poll threads; the v4 producer does not await a transaction's sends before committing (V5 must, because its transactional sends are queued onto an internal dispatch chain); only the first transaction of a V5 test thread waits out the coordinator's asynchronous connect, so the rollover loop still counts every failed open. `--jsse-provider` / `--jca-provider` are also wired into the v4 client builder, which previously read them only for the admin and V5 legs. ### Verifying this change New unit and end-to-end tests in `pulsar-testclient` cover subcommand registration and naming, conf-file defaults and case-insensitive enums on the v4 commands, the shared `-st` enum mapping onto the v4 client enum, produce / consume / read round trips through the v4 commands, the `lid:eid` start position, and the v4-only producer knobs. `PerfToolTest` gains v4 cases that run the commands from `bin/pulsar-perf` in a container.
lhotari
added a commit
to lhotari/pulsar
that referenced
this pull request
Sep 7, 2026
…5 ones ### Motivation `pulsar-perf` was migrated to the V5 client API in apache#25887 (and apache#25917 did the same for `pulsar-client`), so every perf subcommand now drives the V5 SDK. That leaves no way to benchmark the v4 client, or v4 (non-scalable) topics, without the V5 SDK in the path — and several v4 capabilities became unreachable, because they have no V5 equivalent: - `--max-outstanding` / `--max-outstanding-across-partitions` and round-robin partition routing on the producer; - real `Exclusive` / `Failover` / `Key_Shared` subscription types, `MessageListener` dispatch on the client's listener threads, pooled messages, batch-index acknowledgment, the chunked-message knobs, the receiver-queue limits and the auto-scaled receiver-queue reporting on the consumer; - the v4 `Reader` itself — `read` measures a V5 `CheckpointConsumer`, a different broker-side entity — along with a `lid:eid` start message id, `--receiver-queue-size` and `--use-tls`; - the v4 transaction coordinator, which stays a live broker code path next to the v5 one (PIP-473 P5.4) with nothing driving it, and its acknowledgement round-trip latency (V5's `acknowledge` is a synchronous void, so its reported ack latency is a local measurement). ### Modifications Each benchmark is split into an abstract base holding everything that is not client-specific — the CLI options, the throughput/latency accounting, the run() skeleton and message loop, and the reports — plus two thin subclasses that bind the client types: the existing V5 command, and a new v4 one. PerformanceProducerBase -> PerformanceProducer / PerformanceProducerV4 (produce-v4) PerformanceConsumerBase -> PerformanceConsumer / PerformanceConsumerV4 (consume-v4) PerformanceReaderBase -> PerformanceReader / PerformanceReaderV4 (read-v4) PerformanceTransactionBase -> PerformanceTransaction / PerformanceTransactionV4 (transaction-v4) This is a split rather than a revert, so the bases keep the improvements made since the migration — instance recorders, the 3-significant-digit histograms and the latency clamps from apache#26466, class-based subcommand registration from apache#26467 — and both commands share one benchmark and one set of measurements. The v4 commands offer the same flags as their V5 counterparts (only the V5-only `--scalable-consumer-type` and `--scalable` are not mirrored). Each base logs through a logger named after the concrete subclass, so the report lines keep naming the subcommand that produced them. Where the two clients genuinely differ, the seam is explicit: the v4 consumer and reader keep `MessageListener`/`ReaderListener` dispatch while V5 keeps its poll threads; the v4 producer does not await a transaction's sends before committing (V5 must, because its transactional sends are queued onto an internal dispatch chain); only the first transaction of a V5 test thread waits out the coordinator's asynchronous connect, so the rollover loop still counts every failed open. `--jsse-provider` / `--jca-provider` are also wired into the v4 client builder, which previously read them only for the admin and V5 legs. ### Verifying this change New unit and end-to-end tests in `pulsar-testclient` cover subcommand registration and naming, conf-file defaults and case-insensitive enums on the v4 commands, the shared `-st` enum mapping onto the v4 client enum, produce / consume / read round trips through the v4 commands, the `lid:eid` start position, and the v4-only producer knobs. `PerfToolTest` gains v4 cases that run the commands from `bin/pulsar-perf` in a container.
2 of 14 tasks
Radiancebobo
pushed a commit
to Radiancebobo/pulsar
that referenced
this pull request
Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
PulsarPerfTestTool.initCommanderreflectively instantiates every registered subcommand before a single argument is parsed:Constructor.newInstanceis a class-initialization trigger, sopulsar-perf produceruns the static initializers ofconsume,read,transaction,managed-ledger,monitor-brokers,websocket-producerandgen-doctoo. Those classes hold their latencyRecorders in static fields, so aproducerun allocates their HdrHistogram counts arrays and never touches them again. On a heap dump of apulsar-perf producerun this accounted for roughly 170 MB.This is a regression, not long-standing behaviour. Before #22388 (PIP-343, May 2024)
bin/pulsar-perfdispatched to one main class per subcommand:so only the selected command was ever loaded. The subcommand refactor kept the dispatch table but made it eager.
picocli does not require instances:
addSubcommand(String, Class)builds the command spec from annotations and defers creating the user object until that subcommand is actually selected.Modifications
Classobjects rather than pre-built instances, and drop the now-unused reflective instantiation and theaddCommandhelper.setDefaultValueProviderto after the subcommands are registered, and setsetCaseInsensitiveEnumValuesAllowed(true)on the root commander there as well. Both settings propagate to the subcommand hierarchy as it exists at the moment they are invoked. The enum setting previously came fromCmdBase's constructor, which applied it to the per-commandCommandLinethatCmdBasebuilds for itself — that instance is no longer the one picocli parses with, so it has to be set on the root.@Spec CommandSpec specfield fromManagedLedgerWriter,CmdGenerateDocumentationandPerformanceClient. Registering the class is not sufficient for these three: picocli must create the user object to inject a@Specfield, so they stayed eager while the five commands without@Specbecame lazy. All three used the injected spec only to reachspec.commandLine(), and that returned exactly theCommandLineCmdBasebuilds in its own constructor — because the old code registered((CmdBase) o).getCommander()as the subcommand. CallinggetCommander()directly is the same object the code was already using.Net effect on class initialization for
pulsar-perf <conf> produce --help, measured with-Xlog:class+init:PerformanceProducer(the selected one)PerformanceConsumerPerformanceReaderPerformanceTransactionBrokerMonitorManagedLedgerWriterCmdGenerateDocumentationPerformanceClientpulsar-perf producenow initializesPerformanceProducerand no other command class. On currentmasterthat stops roughly 170 MB of histogram allocation (PerformanceConsumer2 x 14,680,080 B,PerformanceReader2 x 14,680,080 B,PerformanceTransaction4 x 22,020,112 B,ManagedLedgerWriter2 x 11,534,352 B). Note that #26466, which shrinks those same histograms about 100x, reduces the size of this particular win considerably if it lands first — the lasting value here is that unused subcommands stop doing any startup work, not just histogram allocation.Verifying this change
This change is already covered by existing tests (
GenerateDocumentionTest,PerformanceBaseArgumentsTest,PerformanceClientTest,PerfClientUtilsTest— the fullpulsar-testclientsuite passes), and was additionally verified by running the real CLI before and after against the same classpath and config file:--helpsubcommand listproduce --helpdefaults sourced from the conf filepulsar://<from conf>:6650produce -z lz4(constant isLZ4)consume -st shared(constant isShared)produce -z nosuchcodecInvalid value for option '--compression': expected one of [NONE, LZ4, ZLIB, ZSTD, SNAPPY] (case-insensitive) but was 'nosuchcodec'Unmatched argument at index 0: 'nosuchcommand'Plus, for the
@Specremoval, the three code paths that used the injected spec:managed-ledgerwith a bad argument -> error + usagewebsocket-producerwith a missing required parameter -> usagegen-doc-> generated tablesThe full captured output of every probe above was diffed between
masterand this branch: the only differences are the class-initialization lines../gradlew quickCheckand./gradlew sanityCheckboth pass.The case-insensitive enum probes matter specifically because that setting moved from
CmdBaseto the root commander; the invalid-enum message still reporting "(case-insensitive)" confirms it is still in effect on the subcommand.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes