gortiz opened a new pull request, #20149: URL: https://github.com/apache/pinot/pull/20149
## Summary - **What changes:** the JVM owns the JFR repository. `ContinuousJfrStarter` only starts, stops and retunes the recording from the `pinot.jfr.*` cluster configs. A new key, `pinot.jfr.instanceRegex`, selects the instances that record. - **Why:** the runtime repository move and the in-JVM cleanup thread are fragile, and MBean `jfrStop` deletes the recorded data. The live control through cluster config is useful, so it stays. - **How:** use the `jdk.jfr` API instead of the DiagnosticCommand MBean, remove the repository options and the cleanup thread, and register one starter per component with its instance ID. This replaces #19372, which moved the whole recording to JVM flags and deprecated the class. ## Background JFR has two kinds of options: | Options | Scope | Owner after this PR | |---|---|---| | `repository`, `preserve-repository`, `dumppath`, `maxchunksize` | the JVM | `-XX:FlightRecorderOptions` | | `settings`, `name`, `maxsize`, `maxage`, `disk`, `dumponexit` | one recording | `pinot.jfr.*` cluster configs, live | Today the class also sets the JVM-global options at runtime, through `jfrConfigure`. That has three problems: 1. The JVM already created a repository before the class moves it. 2. The cleanup thread runs inside the JVM, so it starts only after the JVM is up, and it has to find the repository path by parsing the `jfrConfigure` output. 3. `jfrStop` without a file name deletes the recorded chunks. Every config change restarts the recording, so every config change loses the data. When a deployment sets the repository on a persistent volume with `-XX:FlightRecorderOptions:repository=<dir>,preserve-repository=true`, the chunks of a JVM that crashed or was killed stay on disk. They can be read after the restart. The cleanup of old repositories belongs outside the JVM, before it starts. ## Changes - Use `jdk.jfr.Recording`. Settings, `maxSize`, `maxAge` and `dumpOnExit` change on the running recording. Only a `toDisk` change starts a new recording. - Stop with `Recording.stop()`, which keeps the chunks on disk. `Recording.close()` deletes them. The class keeps the last stopped recording for each name open, and closes the one before it, so the disk usage stays bounded. - Remove `applyRuntimeOptions`, the `pinot-jfr-repository-cleaner` thread, and the `directory`, `dumpPath`, `preserveRepository` and `repositoryMaxTotalSize` keys. If a cluster still sets one of them, the class logs a warning that names the JVM option that replaces it. - Add `pinot.jfr.instanceRegex`. An instance records when `pinot.jfr.enabled` is `true` and the regex matches its **full** instance ID. If the key is not set, all instances record, as before. If the regex is not valid, the instance logs an error and keeps its current state. - If the JVM starts a recording with `-XX:StartFlightRecording`, the class does nothing. `-XX:FlightRecorderOptions` alone does not disable it. - The broker, server, controller and minion starters register `new ContinuousJfrStarter(instanceId)` instead of the `INSTANCE` singleton. Components in one JVM (quickstarts, tests) still share one recording, through the reference count in `GLOBAL_RECORDINGS`. Examples for `pinot.jfr.instanceRegex`: | Target | Value | |---|---| | All servers | `Server_.*` | | One server | `Server_pinot-server-3\..*` | | Brokers and controllers | `(Broker\|Controller)_.*` | ## Upgrade notes `pinot.jfr.directory`, `pinot.jfr.dumpPath`, `pinot.jfr.preserveRepository` and `pinot.jfr.repositoryMaxTotalSize` are no longer read. Replace them with JVM options: ``` -XX:FlightRecorderOptions:repository=<dir>,preserve-repository=true,dumppath=<dir> ``` Put all JFR options in **one** `-XX:FlightRecorderOptions` flag. The JVM does not merge two of these flags: the last one replaces the first one completely. Old repositories under the former `directory` path are not deleted by Pinot anymore. ## Validation - `ContinuousJfrStarterTest`: 19 tests against the JFR of the test JVM, not a mocked MBean. They cover in-place retune, the `toDisk` restart, kept stopped recordings, full-match regex, an invalid regex, the `-XX:StartFlightRecording` guard, shared recordings across components, and rename. - `pinot-core`, `pinot-broker`, `pinot-server`, `pinot-minion`, `pinot-controller` and `pinot-tools` build with checkstyle, spotless and license checks. - Measured on JDK 25: a JVM started with `-XX:FlightRecorderOptions:repository=<dir>,preserve-repository=true` and killed with `SIGKILL` leaves its repository and chunks. The next JVM creates a new repository next to it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
