[PR] [improve][test] Split PulsarProfilingTest into v5 and v4 variants [pulsar]

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

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

   ### Motivation
   
   The profiling sample test could only benchmark one client generation at a time. Switching between the
   v5 scalable topic path and the classic v4 path meant editing the topic domain and the `pulsar-perf`
   subcommands in place, so comparing the two required a working-tree change between runs and the two
   configurations kept overwriting each other.
   
   The two are not independent axes. The v4 client rejects the `topic://` and `segment://` domains
   outright — `PulsarClientImpl.isScalableDomain` fails producer, consumer and reader creation with an
   `InvalidTopicNameException` — so a scalable topic can only be driven by the v5 client. "v5 topic" and
   "v4 topic" are therefore one choice, worth naming once per test class.
   
   Two further problems surfaced while splitting it, both of which made the existing test misreport:
   
   - The print-stats container built its admin URL as `/admin/v2/` plus the topic name with `://`
     replaced by `/`. That works for `persistent://`, but no JAX-RS resource is registered at
     `/admin/v2/topic`: scalable topics are served by `ScalableTopics`, `@Path("/scalable")`. Since the
     test drove a `topic://` topic, its whole topic stats loop was writing 404 bodies into
     `stats.<ts>.txt` and `internal_stats.<ts>.txt`.
   - The run asked for 100,000,000 messages, which takes about 350 seconds (~290k msg/s across produce
     and consume), against a three-minute wait. The run could never finish inside its window, so the
     wait always ended in the `TimeoutException` branch that logs and swallows it — and the exit-code
     assertions that follow then ran against futures that were still incomplete, failing every ordinary
     profiling run. Because the still-running consume future was asserted first, a produce that really
     had died even reported "consume should have completed successfully" and named the wrong command.
   
   ### Modifications
   
   - Rename `PulsarProfilingTest` to `AbstractPulsarProfilingTest` and keep everything shared there: the
     cluster spec, the broker and bookie tuning, the `pulsar-perf` containers and the profiling wiring.
   - Add three hooks for the variation. `getTopicDomain()` and `getPerfCommandSuffix()` pick the topic
     domain and the client generation; the suffix is threaded into `PulsarPerfContainer`, which builds
     `pulsar-perf produce<suffix>` / `consume<suffix>`. `getTopicStatsEndpoints(topicName)` returns the
     admin endpoints to poll as (output file prefix, path) pairs, since each domain is served by a
     different resource and they do not expose the same operations. Returning an empty list collects
     broker metrics only; the metrics scrape is unconditional in every case.
   - `PulsarProfilingTest` drives a v5 scalable (`topic://`) topic with `produce`/`consume` and polls
     `/admin/v2/scalable/<ns>/<topic>/stats`, whose `ScalableTopicStats` already carries the per-segment
     and per-subscription breakdown. There is no `internalStats` counterpart for scalable topics, so the
     managed-ledger internals of the backing segments are not collected.
   - `PulsarProfilingV4Test` drives a classic `persistent://` topic with `produce-v4`/`consume-v4` and
     keeps both `/stats` and `/internalStats` under `/admin/v2/persistent`, exactly as before.
   - Each concrete class declares its own `@Test` delegating to the base's `runPulsarPerfBenchmark()`.
     Gradle's TestNG detector never scans method annotations on an abstract class, so an `@Test` left on
     the base makes both subclasses look like non-test classes and the run fails with "No tests found
     for given includes". (The detector does walk the superclass chain; it is abstractness, not
     inheritance, that stops it.) This matches the existing convention for abstract bases in
     `tests/integration`, such as `TcMetadataDiscoveryTestBase`.
   - Drop the message count to 20,000,000 — a bit over a minute of load at the throughput the containers
     sustain, which leaves ample headroom in the three-minute wait — and remove the `TimeoutException`
     special case, so a run that does not finish now fails. Both commands are complete when the wait
     returns, so the exit-code assertions are unconditional again. A non-zero exit completes the future
     normally with the exit code as its value (`DockerUtils.runCommandAsyncWithLogging`), so those
     assertions remain the only thing that catches a `pulsar-perf` that died early.
   - Both variants write into `build/pulsar-profiling`, so the v4 run's `pulsar-perf` output, histograms,
     topic stats and metrics scrapes are tagged with the same `-v4` suffix. The v5 run keeps the existing
     file names.
   - Update `CONTRIBUTING.md`'s profiling section, which described a single profiling test: document both
     variants and how to run the v4 one, why the client generation and the topic domain are not
     independent, where each run's artifacts land, and that the runs are not like-for-like because
     scalable topics auto-split under load (`scalableTopicAutoScaleEnabled` defaults to true), so the v5
     run profiles a topology that reshapes itself while the v4 run's stays fixed.
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   This change is a modification to a manual profiling test, which is skipped unless
   `pulsar.test.enableManualTest` is set, so CI does not exercise it. It was verified locally:
   
   - `./gradlew :tests:integration:integrationTest --tests "*PulsarProfilingTest*" --tests "*PulsarProfilingV4Test*"`
     discovers and runs both concrete classes (both skip on the manual-test guard) and does not pick up
     the abstract base.
   - `./gradlew :tests:integration:profilingIntegrationTest` still resolves its hardcoded filter to
     `PulsarProfilingTest` alone; the v4 variant runs with `--tests "*PulsarProfilingV4Test"`.
   - The generated stats scripts were rendered for both variants and for the metrics-only case and
     checked with `bash -n`; the v4 script is unchanged from the previous behaviour.
   - The exit-code assertions were exercised against all six outcomes: both commands running, one
     running, both exited 0, produce exited non-zero with and without consume still running, and a
     docker-layer failure.
   - `./gradlew spotlessCheck checkstyleMain checkstyleTest` and `./gradlew quickCheck` pass.
   
   ### 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
   


-- 
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] Split PulsarProfilingTest into v5 and v4 variants [pulsar]

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

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


-- 
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