Copilot commented on code in PR #17851:
URL: https://github.com/apache/pinot/pull/17851#discussion_r2913018251
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java:
##########
@@ -169,24 +173,15 @@ private void stopRecording() {
if (!_running) {
return;
}
- assert _recording != null;
- LOGGER.debug("Stopping recording {}", _recording.getName());
- _recording.stop();
- _recording.close();
-
- if (_cleanupThread != null) {
- _cleanupThread.interrupt();
- try {
- _cleanupThread.join(5_000);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- LOGGER.warn("Interrupted while waiting for cleanup thread to stop");
- }
- _cleanupThread = null;
+ assert _recordingName != null;
+ if (!isDiagnosticCommandAvailable()) {
+ LOGGER.warn("JFR DiagnosticCommand MBean is unavailable. Marking
recording '{}' as stopped in Pinot state only",
+ _recordingName);
+ } else {
+ executeDiagnosticCommand(JFR_STOP_COMMAND, "name=" + _recordingName);
+ LOGGER.info("Stopped continuous JFR recording {}", _recordingName);
}
Review Comment:
`stopRecording()` unconditionally clears `_recordingName` and sets `_running
= false` even if the stop command fails (or if the MBean is unavailable). This
can leave an actual JVM recording running while Pinot believes it is stopped,
and can also lead to conflicts if a later enable tries to `jfrStart` with the
same name. Consider only updating Pinot state after a successful `jfrStop`
(check the boolean return), and if unavailable/failed, either keep
`_running`/`_recordingName` so Pinot can retry later, or actively verify stop
success via `jfrCheck` before clearing state.
```suggestion
LOGGER.warn("JFR DiagnosticCommand MBean is unavailable. Cannot stop
continuous JFR recording '{}'",
_recordingName);
// Keep _running and _recordingName so that a future attempt can retry
stopping the recording.
return;
}
boolean stopped = executeDiagnosticCommand(JFR_STOP_COMMAND, "name=" +
_recordingName);
if (!stopped) {
LOGGER.warn("Failed to stop continuous JFR recording '{}'",
_recordingName);
// Do not clear state; the recording may still be running.
return;
}
LOGGER.info("Stopped continuous JFR recording {}", _recordingName);
```
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java:
##########
@@ -169,24 +173,15 @@ private void stopRecording() {
if (!_running) {
return;
}
- assert _recording != null;
- LOGGER.debug("Stopping recording {}", _recording.getName());
- _recording.stop();
- _recording.close();
-
- if (_cleanupThread != null) {
- _cleanupThread.interrupt();
- try {
- _cleanupThread.join(5_000);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- LOGGER.warn("Interrupted while waiting for cleanup thread to stop");
- }
- _cleanupThread = null;
+ assert _recordingName != null;
+ if (!isDiagnosticCommandAvailable()) {
+ LOGGER.warn("JFR DiagnosticCommand MBean is unavailable. Marking
recording '{}' as stopped in Pinot state only",
+ _recordingName);
+ } else {
+ executeDiagnosticCommand(JFR_STOP_COMMAND, "name=" + _recordingName);
+ LOGGER.info("Stopped continuous JFR recording {}", _recordingName);
Review Comment:
There’s no test coverage for the scenario where
`executeDiagnosticCommand(...)` returns `false` for `jfrStop`. Given the
state-management risk, add a unit test that forces
`executeDiagnosticCommand(JFR_STOP_COMMAND, ...)` to fail and asserts Pinot’s
running state and recording name handling is correct (e.g., remains running for
retry, or only clears after verifying stop).
--
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]
