Enhance ContinuousJfrStarter to utilize JFR DiagnosticCommand MBean for recording management#17851
Conversation
…or recording management
There was a problem hiding this comment.
Pull request overview
This PR updates Pinot’s continuous JFR integration to control recordings via the JVM DiagnosticCommand MBean (jfrConfigure, jfrStart, jfrStop) rather than the jdk.jfr.Recording API, while preserving cluster-level dynamic control through ContinuousJfrStarter.
Changes:
- Replace
RecordingAPI usage with MBean command execution for start/stop/configure. - Add runtime option mapping for repository and dump paths and simplify lifecycle (remove dump cleanup thread logic).
- Update tests to validate generated commands and add an integration-style test using the real DiagnosticCommand MBean.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java | Switch continuous JFR control to DiagnosticCommand MBean, add repo/dump path configuration, remove dump cleanup thread. |
| pinot-core/src/test/java/org/apache/pinot/core/util/trace/ContinuousJfrStarterTest.java | Update unit tests to assert executed MBean commands and add a real-MBean integration test. |
Comments suppressed due to low confidence (1)
pinot-core/src/test/java/org/apache/pinot/core/util/trace/ContinuousJfrStarterTest.java:1
- Multiple tests assert the full rendered command string (including argument ordering). This makes the suite brittle to harmless internal changes (e.g., reordering arguments) while still producing semantically equivalent commands. Consider asserting on command type plus a set/contains of key arguments (or parsing into a canonical representation) so tests fail only on meaningful command changes.
/**
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17851 +/- ##
============================================
- Coverage 63.23% 63.21% -0.03%
- Complexity 1466 1525 +59
============================================
Files 3190 3193 +3
Lines 192009 193488 +1479
Branches 29412 29760 +348
============================================
+ Hits 121424 122305 +881
- Misses 61063 61583 +520
- Partials 9522 9600 +78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @VisibleForTesting | ||
| protected static Path getRecordingPath(Path parentDir, String name, Instant timestamp) { | ||
| String filename = "recording-" + name + "-" + timestamp + ".jfr"; | ||
| return parentDir.resolve(filename); | ||
| } | ||
|
|
||
| private void prepareFileDumps(PinotConfiguration subset) { | ||
| protected boolean executeDiagnosticCommand(String operationName, String... arguments) { | ||
| try { | ||
| Path directory = Path.of(subset.getProperty(DIRECTORY, Paths.get(".").toString())); | ||
| if (!directory.toFile().canWrite()) { | ||
| throw new RuntimeException("Cannot write: " + directory); | ||
| } | ||
| _mBeanServer.invoke(_diagnosticCommandObjectName, operationName, new Object[]{arguments}, | ||
| new String[]{String[].class.getName()}); | ||
| return true; | ||
| } catch (Exception e) { | ||
| LOGGER.warn("Failed to execute JFR command '{}' with arguments {}", operationName, Arrays.toString(arguments), e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
executeDiagnosticCommand() can be invoked with a null _diagnosticCommandObjectName (e.g., if createDiagnosticCommandObjectName() returned null) and will then rely on catching a resulting exception. Adding an explicit null/availability guard here (and logging a clearer one-line message) would avoid noisy stack traces and make the failure mode clearer for callers/subclasses.
| private static boolean waitForRecordingPresence(String recordingName, boolean expectedPresent, int maxAttempts, | ||
| long delayMs) { | ||
| for (int i = 0; i < maxAttempts; i++) { | ||
| boolean present = isRecordingPresent(recordingName); | ||
| if (present == expectedPresent) { | ||
| return true; | ||
| } | ||
| try { | ||
| Thread.sleep(delayMs); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| return false; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
This integration test uses a custom Thread.sleep polling loop with a 1s total wait (10 * 100ms), which is prone to CI flakiness on slow or loaded machines. Prefer using the existing TestUtils.waitForCondition(...) helper (used elsewhere in pinot-core tests) with a more generous timeout and failure message.
| String recordingName = "pinot-continuous-it-" + System.currentTimeMillis(); | ||
| Map<String, String> enabledConfig = Map.of( | ||
| "pinot.jfr.enabled", "true", | ||
| "pinot.jfr.name", recordingName, | ||
| "pinot.jfr.toDisk", "false"); | ||
| Map<String, String> disabledConfig = Map.of( | ||
| "pinot.jfr.enabled", "false", | ||
| "pinot.jfr.name", recordingName, | ||
| "pinot.jfr.toDisk", "false"); |
There was a problem hiding this comment.
The real-MBean integration test only exercises toDisk=false, so it won't catch incompatibilities in generated arguments like maxage/maxsize (which are only sent when toDisk=true). Consider adding an additional integration assertion that starts a recording with toDisk=true and a non-default maxAge to validate that toJfrTimeArgument(...) produces a value accepted by the DiagnosticCommand MBean.
yashmayya
added
the
observability
yashmayya
left a comment
yashmayya
left a comment
There was a problem hiding this comment.
Nice, this seems like a much cleaner approach
| public static final String DEFAULT_MAX_SIZE = "2GB"; | ||
| /// Key that controls the maximum age of the recording file. | ||
| /// Once the file reaches this age, older events will be discarded. | ||
| /// If both maxSize and maxAge are set, the recording will be discarded when either condition is met. | ||
| /// | ||
| /// This is only used if toDisk is true. | ||
| /// The value is a duration string, as defined by the Duration class. | ||
| /// The default value is 1 day (P1D). | ||
| /// The default value is 7 days (P7D). |
There was a problem hiding this comment.
Curious - why these larger defaults?
| return true; | ||
| } | ||
| try { | ||
| Thread.sleep(delayMs); |
There was a problem hiding this comment.
nit: can use TestUtils::waitForCondition pattern instead?
| import org.assertj.core.api.Assertions; | ||
| import org.mockito.Mockito; | ||
| import org.testng.annotations.BeforeMethod; | ||
| import org.testng.SkipException; |
There was a problem hiding this comment.
This is causing the spotless plugin check to fail
| private void cleanupRepositoriesIfNeeded(String recordingName, String repositoryRoot, long maxTotalSizeBytes) { | ||
| try { | ||
| Configuration configuration = Configuration.getConfiguration(jfrConfName); | ||
| return new Recording(configuration); | ||
| } catch (ParseException e) { | ||
| throw new RuntimeException("Failed to parse JFR configuration '" + jfrConfName + "'", e); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException("Failed to read JFR configuration '" + jfrConfName + "'", e); | ||
| Path repositoryRootPath = Path.of(repositoryRoot).toAbsolutePath().normalize(); | ||
| Path activeRepositoryPath = resolveCurrentRepositoryPath(); | ||
| cleanupOldRepositoriesInternal(repositoryRootPath, maxTotalSizeBytes, activeRepositoryPath); | ||
| } catch (Exception e) { | ||
| LOGGER.warn("Failed to clean up old JFR repositories for recording '{}' under '{}'", recordingName, repositoryRoot, | ||
| e); | ||
| } | ||
| } |
| return; | ||
| @VisibleForTesting | ||
| protected static void resetGlobalStateForTesting() { | ||
| synchronized (GLOBAL_RECORDING_LOCK) { |
| if (preserveRepository) { | ||
| List<String> configureArgumentsWithPreserve = new ArrayList<>(configureArguments); | ||
| configureArgumentsWithPreserve.add("preserve-repository=true"); | ||
| if (executeDiagnosticCommand(JFR_CONFIGURE_COMMAND, configureArgumentsWithPreserve.toArray(new String[0]))) { | ||
| return true; | ||
| } | ||
| LOGGER.warn("Failed to apply preserveRepository option for continuous JFR recording. Retrying without it"); | ||
| } |
| if (!isDiagnosticCommandAvailable()) { | ||
| LOGGER.warn("JFR DiagnosticCommand MBean is unavailable. Cannot start continuous JFR recording"); | ||
| return false; | ||
| } | ||
| if (!applyRuntimeOptions(subset)) { | ||
| LOGGER.warn("Failed to apply JFR runtime options for recording '{}'", recordingName); | ||
| return false; | ||
| } |
| clusterConfigOverrides.put(CommonConstants.JFR + ".dumpPath", jfrDirectory); | ||
| clusterConfigOverrides.put(CommonConstants.JFR + ".preserveRepository", "true"); |
|
Documentation PR: pinot-contrib/pinot-docs#572 This PR updates the Continuous JFR runbook to reflect the DiagnosticCommand MBean integration changes in this PR. |
…pache/pinot#17851) (#572) Co-authored-by: Claude Code Agent <claude@anthropic.com>
Summary
This PR reworks Pinot's continuous JFR integration to use the JVM DiagnosticCommand MBean (
JFR.configure,JFR.start,JFR.stop) instead of thejdk.jfr.Recordingobject API, while keeping cluster-level dynamic control throughContinuousJfrStarter.Why this change is needed
The previous implementation had correctness and operability issues:
Recording.setDestination(...), which does not match the intended long-running "continuous recording + operator-managed dumps" behavior.The new approach aligns Pinot with JFR's actual control model and removes behavior that was easy to misinterpret.
Why we decided not to keep using
RecordingWe moved away from
RecordingAPI for control flow because important operational knobs we need are in the global JFR control plane, not the per-recording API:repositorypath)dumppath)Using DiagnosticCommand MBean gives one coherent mechanism for all required controls and maps directly to
jcmdbehavior.Why we still keep
ContinuousJfrStarter(instead of onlyjcmdorJAVA_OPTS)We intentionally keep
ContinuousJfrStarteras Pinot's control plane because it gives cluster-wide operability thatJAVA_OPTS/manualjcmddo not:JAVA_OPTSand directjcmdremain valid tools, but they are not sufficient as the primary Pinot control plane for large clusters.What changed
ContinuousJfrStarterbehaviorjfrConfigurefor runtime options (repositorypath,dumppath)jfrStartfor recording startjfrStopfor stop by recording namepinot.jfr.directory->repositorypath(kept for backward compatibility)pinot.jfr.dumpPath->dumppathenabled,name,configuration,toDisk,maxSize,maxAge,dumpOnExitReliability hardening
ObjectNamecreation fails (logs warning; does not throw).Tests
jfrCheck.Docs (See pinot-contrib/pinot-docs#471)
pinot.jfr.directorycompatibility key andpinot.jfr.dumpPath