[PR] [improve][test] Separate IoT warmup from performance measurements [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari opened a new pull request, #26667:
URL: https://github.com/apache/pulsar/pull/26667

   ### Motivation
   
   Performance recordings currently include workload startup and the first traffic sent through newly initialized clients, connections, producers, and consumers. Periodic managed-ledger rollover and BookKeeper maintenance can also land inside a short measurement interval. These effects make throughput summaries and JFR comparisons sensitive to work outside the broker messaging path being studied.
   
   Keeping the complete recording is useful for diagnosing startup and shutdown behavior, while a recording restricted to the measured workload makes repeatable profile and differential-flamegraph comparisons easier. Selecting that interval manually from epoch timestamps is error-prone.
   
   ### Modifications
   
   - Add mutually exclusive `warmupSeconds` and `warmupMessages` settings to the IoT workload.
   - Add repeatable warmup rounds and a configurable delay after each fully drained round, including the final round before measurement. A shared barrier waits until every backend application has uniquely received the cumulative round total; producer send completions alone cannot advance the phase. Rate limiting is restarted after the delay so it does not create a catch-up burst.
   - Keep the same clients, connections, producers, consumers, and monotonic device sequences across warmup and measurement.
   - Include warmup messages in end-to-end delivery, duplicate, and ordering validation while excluding their completions from measured producer throughput.
   - Record producer measurement boundaries in `producer-summary.json` and each backend application’s first and last measured-message receipt in `consumer-summary.json`. The first receipt is descriptive metadata; only the latest last receipt controls the cut end.
   - By default, retain the complete async-profiler JFR and create a second `.measurement.jfr` spanning the producer measurement start through the latest measured-message receipt across all backend applications. The cut includes the full millisecond containing the final receipt and retains one-time JVM, host, recording setting and runtime configuration events so JDK Mission Control can describe the source JVM. Either output can be disabled independently.
   - Add a reusable `JfrCut` Java API and a `runJfrCut` Gradle task. Boundaries accept ISO-8601 timestamps, epoch milliseconds, and recording-relative offsets such as `5s`. Either boundary can be omitted. `--info` reads the actual source recording start, end, and wall-clock duration from the JFR chunk headers; cut recordings preserve those source timestamps for tools such as JDK Mission Control.
   - Record successful measured-message producer send-completion latency and backend-application broker-publish-to-listener latency as HdrHistogram logs. A payload marker excludes warmup from both, and consumer latency is sampled before validation, acknowledgment, or simulated processing.
   - Add `renderHdrHistograms` to merge backend-application histograms by observation count and render producer and consumer latency distributions as PNG and SVG without external plotting tools.
   - Keep topic and cursor managed-ledger rollover outside ordinary benchmark windows by raising their entry, size, and time limits.
   - Delay BookKeeper ledger garbage collection, disable entry-log compaction, and raise the journal size limit for the ephemeral benchmark cluster. Normal entry-log flushing and disk-space checks remain enabled.
   
   The storage values are benchmark controls rather than production sizing recommendations. Scenarios intended to measure rollover, recovery, deletion, compaction, or long-running storage behavior should override them.
   
   ### Validation
   
   - `./gradlew :tests:performance:tools:test :tests:performance:launcher:test quickCheck --no-daemon`
   - `./gradlew spotlessCheck checkstyleMain checkstyleTest --no-daemon`
   - A profiled container smoke scenario completed two warmup rounds with a delay after each, then measured 200 messages at 100 msg/s. All 400 messages were received with zero duplicates, ordering violations, or invalid payloads.
   - The smoke run produced complete and measurement-window broker and producer JFRs. The `runJfrCut` task also produced a valid JFR from the same recording using a relative `--from 1s` boundary with the end omitted.
   - A small end-to-end container run sent 50 warmup and 200 measured messages. The producer and consumer HDR logs each contained exactly 200 observations, and `renderHdrHistograms` generated valid PNG and SVG distributions.
   - A two-round container run advanced its warmup barriers at 50 and 100 uniquely received messages, then completed 200 measured messages. Both consumer receipt timestamps and both per-round application markers were present.
   
   ### Does this pull request potentially affect one of the following parts:
   
   - [ ] 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
   
   
   
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][test] Separate IoT warmup from performance measurements [pulsar]

["nodece (via GitHub)" <gi...@apache.org>]

nodece commented on PR #26667:
URL: https://github.com/apache/pulsar/pull/26667#issuecomment-5754843258

   Thanks for this well-structured change. I reviewed the full diff (all 24 files), the warmup/measurement barrier logic, and verified the JFR handling against a real recording on JDK 21. Overall the design is sound: the warmup/measurement separation is consistent across producer, consumer, summaries, HDR logs, and the JFR cut, and the barrier correctly waits for every application's unique receipt count rather than producer send completions.
   
   A few issues worth addressing:
   
   1. **Cross-container clock assumption for the JFR cut window.** The cut start uses the producer container's `measurementStartEpochMs` and the end uses the latest consumer last-receipt timestamp, while JFR event timestamps come from each process's own clock (broker/consumer/producer run in separate containers). This is fine when all containers share the host clock, but on multi-host or skewed-clock setups the broker JFR window will be offset. Consider documenting the assumption (or aligning on broker-side timestamps).
   
   2. **`retainOriginalRecording: false` + `createMeasurementRecording: false` deletes every recording.** `JfrRecordingProcessor.process` deletes the originals while creating nothing, and nothing validates against disabling both flags. Consider rejecting that combination in the launcher.
   
   3. **Barrier marker write failures are treated as invalid messages.** `markCompletedWarmupRounds` throws (e.g. IOException while writing the marker), and the listener's `catch (RuntimeException)` counts it as `invalidMessage` and nacks the message → repeated redelivery and repeated failed marker writes. Infrastructure errors should be handled separately from message-validation errors.
   
   4. **`--coordination-directory` became a required option for all tool commands.** This breaks direct CLI invocation of `iot-produce`/`iot-consume` outside the launcher. A default (e.g. under `--output`) or a per-command requirement would keep direct usage working.
   
   Minor nits:
   
   - The device-selection loop (`while (!devicesInFlight.add(device))`) busy-spins when in-flight == `maxOutstanding`; acquiring a permit before selecting the device would avoid it.
   - A consumer that times out before receiving any measurement message fails with an IOException from the empty HDR write instead of exiting with code 1 and leaving diagnostics.
   - Re-running into the same output directory leaves stale originals/`.measurement.jfr` files, and `retainOriginalRecording: false` won't clean them.
   - Docs say half-open `[from, to)` but the filter is `start < to && end >= from`; consumer latency is also sampled after decode/key validation, slightly later than "before validation" implies.
   
   *Note: this review was prepared with assistance from an AI coding assistant and reviewed by a human before posting, per the ASF generative tooling guidance.*
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][test] Separate IoT warmup from performance measurements [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari commented on PR #26667:
URL: https://github.com/apache/pulsar/pull/26667#issuecomment-5756270925

   @nodece, thanks for the review. I've addressed the issues that affect ordinary use and clarified the assumptions in the documentation. This is testing and profiling code for controlled experiments, so I would keep the safeguards proportionate to that use. The person running an experiment can choose unusual settings and is responsible for interpreting the results; we don't need to reject every configuration that could produce an unhelpful run.
   
   Responding to the points in [your review](https://github.com/apache/pulsar/pull/26667#issuecomment-5754843258), with short excerpts below:
   
   > Cross-container clock assumption
   
   Documented that the JFR cut window and broker-publish-to-listener latency assume synchronized clocks. The launcher runs the containers on one Docker host, where they share the host clock. Someone adapting the tools to multiple hosts needs to synchronize those clocks. I don't think clock-skew estimation or broker-side boundary coordination is warranted for this harness.
   
   > deletes every recording
   
   I've kept both retention flags independent and documented that setting both to `false` discards the recordings produced by that run. Both still default to `true`. An explicit choice to discard profiling output doesn't need to be prohibited in a testing tool. The existing tests cover all four combinations.
   
   > invalid messages
   
   Moved warmup marker writes out of the message listener and into the consumer's control loop. A marker-write failure now fails the command with the filesystem error; it no longer increments the invalid-message count or negatively acknowledges an otherwise valid message. This also removes filesystem I/O and warmup coordination from the listener path.
   
   > required option
   
   `--coordination-directory` is now optional and defaults to `<output>/coordination`. Runs without warmup need neither a coordination option nor a run ID.
   
   For warmup, the launcher generates a UUID, saves it in `run-id.txt`, and passes it as `--run-id` to the producer and every consumer. Barrier filenames contain that ID, the round, and the application index. Old markers can remain in the directory without releasing a later run's barrier, so the launcher no longer deletes previous markers.
   
   Direct warmup invocations supply the same fresh `--run-id` to all participants. When their output directories differ, they also supply a common `--coordination-directory`. The documentation includes an example, and tests cover direct invocation and isolation from previous runs and other rounds.
   
   > busy-spins
   
   The producer now acquires an outstanding-send permit before selecting a device. The permit count is capped by the number of devices, since the workload allows only one in-flight message per device. Thus, when every device is busy, the producer waits for a completion before trying to select one. Random selection can still retry occupied devices when capacity is available.
   
   > empty HDR write
   
   Empty histograms can now be written. A consumer that times out without receiving any measured messages can write its state, violation samples, and summary and return status 1, rather than failing first on the histogram write. Added an empty-histogram round-trip test.
   
   > stale originals/`.measurement.jfr` files
   
   I've documented that retention flags apply to the current run and that a fresh output directory should be used for each experiment. Earlier recordings are deliberately left alone; disabling retention for this run should not delete profiles saved by a previous experiment. The correlation ID addresses stale warmup markers without requiring automatic cleanup of historical artifacts.
   
   > half-open `[from, to)`
   
   Corrected the JFR filter: a duration event ending exactly at `from` is excluded, an instantaneous event at `from` is included, and an event starting at `to` is excluded. Added a test using exact timestamps read back from a real JFR recording, including `cutFrom`.
   
   Also clarified the latency wording: the receipt timestamp is captured at listener entry; the histogram sample is recorded after payload decoding and key validation, before sequence validation and acknowledgment. The latency value therefore excludes decoding and validation time.
   
   Validation: 27 tests passed across the performance tools and launcher, including the new regression cases, and `quickCheck` passed:
   
   ```bash
   ./gradlew :tests:performance:tools:test --tests 'org.apache.pulsar.tests.performance.tools.*' \
     :tests:performance:launcher:test --tests 'org.apache.pulsar.tests.performance.launcher.*' \
     quickCheck -PtestRetryCount=0 --console=plain
   ```
   
   I have not rerun the container smoke scenario for this follow-up.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][test] Separate IoT warmup from performance measurements [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari merged PR #26667:
URL: https://github.com/apache/pulsar/pull/26667


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org