Skip to content

[fix][cli] pulsar-perf: register subcommand classes with picocli instead of instances - #26467

Merged
merlimat merged 2 commits into
apache:masterfrom
lhotari:lh-fix-perf-lazy-subcommands
Sep 4, 2026
Merged

merlimat merged 2 commits into
apache:masterfrom
lhotari:lh-fix-perf-lazy-subcommands

Conversation

@lhotari

@lhotari lhotari commented Sep 4, 2026 •

Copy link
Copy Markdown
Member

Motivation

PulsarPerfTestTool.initCommander reflectively instantiates every registered subcommand before a single argument is parsed:

for (Map.Entry<String, Class<?>> c : commandMap.entrySet()) {
    Constructor<?> constructor = c.getValue().getDeclaredConstructor();
    constructor.setAccessible(true);
    addCommand(c.getKey(), constructor.newInstance());   // <- runs every command's <clinit>
}

Constructor.newInstance is a class-initialization trigger, so pulsar-perf produce runs the static initializers of consume, read, transaction, managed-ledger, monitor-brokers, websocket-producer and gen-doc too. Those classes hold their latency Recorders in static fields, so a produce run allocates their HdrHistogram counts arrays and never touches them again. On a heap dump of a pulsar-perf produce run this accounted for roughly 170 MB.

This is a regression, not long-standing behaviour. Before #22388 (PIP-343, May 2024) bin/pulsar-perf dispatched to one main class per subcommand:

if [ "$COMMAND" == "produce" ]; then
    exec $JAVA $OPTS org.apache.pulsar.testclient.PerformanceProducer --conf-file $PULSAR_PERFTEST_CONF "$@"
elif [ "$COMMAND" == "consume" ]; then
...

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

  • Register the command Class objects rather than pre-built instances, and drop the now-unused reflective instantiation and the addCommand helper.
  • Move setDefaultValueProvider to after the subcommands are registered, and set setCaseInsensitiveEnumValuesAllowed(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 from CmdBase's constructor, which applied it to the per-command CommandLine that CmdBase builds for itself — that instance is no longer the one picocli parses with, so it has to be set on the root.
  • Drop the @Spec CommandSpec spec field from ManagedLedgerWriter, CmdGenerateDocumentation and PerformanceClient. Registering the class is not sufficient for these three: picocli must create the user object to inject a @Spec field, so they stayed eager while the five commands without @Spec became lazy. All three used the injected spec only to reach spec.commandLine(), and that returned exactly the CommandLine CmdBase builds in its own constructor — because the old code registered ((CmdBase) o).getCommander() as the subcommand. Calling getCommander() 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:

class before after
PerformanceProducer (the selected one) 1 1
PerformanceConsumer 1 0
PerformanceReader 1 0
PerformanceTransaction 1 0
BrokerMonitor 1 0
ManagedLedgerWriter 1 0
CmdGenerateDocumentation 1 0
PerformanceClient 1 0

pulsar-perf produce now initializes PerformanceProducer and no other command class. On current master that stops roughly 170 MB of histogram allocation (PerformanceConsumer 2 x 14,680,080 B, PerformanceReader 2 x 14,680,080 B, PerformanceTransaction 4 x 22,020,112 B, ManagedLedgerWriter 2 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

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests (GenerateDocumentionTest, PerformanceBaseArgumentsTest, PerformanceClientTest, PerfClientUtilsTest — the full pulsar-testclient suite passes), and was additionally verified by running the real CLI before and after against the same classpath and config file:

probe before after
--help subcommand list all 8 identical, all 8
produce --help defaults sourced from the conf file pulsar://<from conf>:6650 identical
case-insensitive enum produce -z lz4 (constant is LZ4) parses parses
case-insensitive enum consume -st shared (constant is Shared) parses parses
invalid enum produce -z nosuchcodec Invalid value for option '--compression': expected one of [NONE, LZ4, ZLIB, ZSTD, SNAPPY] (case-insensitive) but was 'nosuchcodec' byte-identical
unknown subcommand Unmatched argument at index 0: 'nosuchcommand' byte-identical

Plus, for the @Spec removal, the three code paths that used the injected spec:

probe before after
managed-ledger with a bad argument -> error + usage error text and full usage block, defaults included byte-identical
websocket-producer with a missing required parameter -> usage usage block byte-identical
gen-doc -> generated tables 7 command sections, 14 headings byte-identical

The full captured output of every probe above was diffed between master and this branch: the only differences are the class-initialization lines. ./gradlew quickCheck and ./gradlew sanityCheck both pass.

The case-insensitive enum probes matter specifically because that setting moved from CmdBase to 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

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

…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)
…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
merlimat merged commit f41e620 into apache:master Sep 4, 2026
44 checks passed
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.
@lhotari lhotari added this to the 5.0.0-M2 milestone Sep 9, 2026
lhotari added a commit that referenced this pull request Sep 9, 2026
lhotari added a commit that referenced this pull request Sep 9, 2026
Radiancebobo pushed a commit to Radiancebobo/pulsar that referenced this pull request Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants