Copilot commented on code in PR #16987:
URL: https://github.com/apache/pinot/pull/16987#discussion_r2415899613
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java:
##########
@@ -118,80 +123,157 @@ public class ContinuousJfrStarter {
/// A flag to track whether the JFR recording has been started.
/// This is specially useful for testing and quickstarts, where servers,
brokers and other components are executed
/// in the same JVM.
- private static boolean _started = false;
+ public static final ContinuousJfrStarter INSTANCE = new
ContinuousJfrStarter();
+ @GuardedBy("this")
+ private boolean _running = false;
+ @GuardedBy("this")
+ private Map<String, Object> _currentConfig;
+ @GuardedBy("this")
+ private Recording _recording;
+ @GuardedBy("this")
+ private Thread _cleanupThread;
- private ContinuousJfrStarter() {
+ @VisibleForTesting
+ protected ContinuousJfrStarter() {
}
- public synchronized static void init(PinotConfiguration config) {
+ @Override
+ public void onChange(Set<String> changedConfigs, Map<String, String>
clusterConfigs) {
+ boolean jfrChanged = changedConfigs.stream()
+ .anyMatch(changedConfig ->
changedConfig.startsWith(CommonConstants.JFR));
+ if (!jfrChanged && _currentConfig != null) {
+ LOGGER.debug("ChangedConfigs: {} does not contain any JFR config.
Skipping updates", changedConfigs);
+ return;
+ }
+ PinotConfiguration config = new PinotConfiguration(clusterConfigs);
PinotConfiguration subset = config.subset(CommonConstants.JFR);
+ synchronized (this) {
+ Map<String, Object> newSubsetMap = subset.toMap();
+
+ if (_currentConfig != null && _currentConfig.equals(newSubsetMap)) {
+ // No change
+ LOGGER.debug("JFR config change detected, but no actual change in
config");
+ return;
+ }
+
+ stopRecording();
+ _currentConfig = newSubsetMap;
+ startRecording(subset);
+ }
+ }
+
+ public boolean isRunning() {
+ return _running;
+ }
+
+ 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;
+ }
+
+ LOGGER.info("Stopped continuous JFR recording {}", _recording.getName());
+ _recording = null;
+ _running = false;
+ }
+
+ private void startRecording(PinotConfiguration subset) {
if (!subset.getProperty(ENABLED, DEFAULT_ENABLED)) {
+ LOGGER.info("Continuous JFR recording is disabled");
return;
}
- if (_started) {
+ if (_running) {
return;
}
+ _recording = createRecording(subset);
+ _recording.setName(subset.getProperty(NAME, DEFAULT_NAME));
+
+ _recording.setDumpOnExit(subset.getProperty(DUMP_ON_EXIT,
DEFAULT_DUMP_ON_EXIT));
+
+ prepareFileDumps(subset);
+
+ try {
+ boolean toDisk = subset.getProperty(TO_DISK, DEFAULT_TO_DISK);
+ if (toDisk) {
+ _recording.setToDisk(true);
+ _recording.setMaxSize(subset.getProperty(MAX_SIZE, DEFAULT_MAX_SIZE));
+ _recording.setMaxAge(Duration.parse(subset.getProperty(MAX_AGE,
DEFAULT_MAX_AGE).toUpperCase(Locale.ENGLISH)));
+ }
+ } catch (DateTimeParseException e) {
+ throw new RuntimeException("Failed to parse duration", e);
+ }
+ _recording.start();
+ LOGGER.info("Started continuous JFR recording {} with configuration: {}",
_recording.getName(), subset);
+ _running = true;
+ }
+
+ private void prepareFileDumps(PinotConfiguration subset) {
+ try {
+ Path directory = Path.of(subset.getProperty(DIRECTORY,
Paths.get(".").toString()));
+ if (!directory.toFile().canWrite()) {
+ throw new RuntimeException("Cannot write: " + directory);
+ }
+
+ String timestamp = ZonedDateTime.ofInstant(Instant.now(), ZoneOffset.UTC)
+ .format(DateTimeFormatter.ofPattern("yyyy-MM-dd_HH-mm-ss"));
+ String filename = "recording-" + _recording.getName() + timestamp +
".jfr";
+ Path recordingPath = directory.resolve(filename);
+ _recording.setDestination(recordingPath);
- Recording recording;
+ int maxDumps = subset.getProperty(MAX_DUMPS, DEFAULT_MAX_DUMPS);
+ if (maxDumps > 0) {
+ _cleanupThread = createThread(() -> cleanUpDumps(directory, maxDumps,
_recording.getName()));
+ _cleanupThread.start();
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to create new recording file", e);
+ }
+ }
+
+ @VisibleForTesting
+ protected Recording createRecording(PinotConfiguration subset) {
String jfrConfName = subset.getProperty(CONFIGURATION,
DEFAULT_CONFIGURATION);
try {
Configuration configuration =
Configuration.getConfiguration(jfrConfName);
- recording = new Recording(configuration);
+ 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);
}
- boolean dumpOnExit = subset.getProperty(DUMP_ON_EXIT,
DEFAULT_DUMP_ON_EXIT);
- recording.setDumpOnExit(dumpOnExit);
- if (dumpOnExit) {
- try {
- Path directory = Path.of(subset.getProperty(DIRECTORY,
Paths.get(".").toString()));
- if (!directory.toFile().canWrite()) {
- throw new RuntimeException("Cannot write: " + directory);
- }
-
- String timestamp = ZonedDateTime.ofInstant(Instant.now(),
ZoneOffset.UTC)
- .format(DateTimeFormatter.ofPattern("yyyy-MM-dd_HH-mm-ss"));
- String filename = "recording-" + timestamp + ".jfr";
- Path recordingPath = directory.resolve(filename);
- recording.setDestination(recordingPath);
-
- int maxDumps = subset.getProperty(MAX_DUMPS, DEFAULT_MAX_DUMPS);
- if (maxDumps > 0) {
- Thread cleanupThread = new Thread(() -> cleanUpDumps(directory,
maxDumps));
- cleanupThread.setName("JFR-Dump-Cleanup");
- cleanupThread.setDaemon(true);
- cleanupThread.start();
- }
- } catch (IOException e) {
- throw new UncheckedIOException("Failed to create new recording file",
e);
- }
- }
+ }
- try {
- recording.setName(subset.getProperty(NAME, DEFAULT_NAME));
- boolean toDisk = subset.getProperty(TO_DISK, DEFAULT_TO_DISK);
- if (toDisk) {
- recording.setToDisk(true);
- recording.setMaxSize(subset.getProperty(MAX_SIZE, DEFAULT_MAX_SIZE));
- recording.setMaxAge(Duration.parse(subset.getProperty(MAX_AGE,
DEFAULT_MAX_AGE).toUpperCase(Locale.ENGLISH)));
- }
- } catch (DateTimeParseException e) {
- throw new RuntimeException("Failed to parse duration", e);
- }
- recording.start();
- _started = true;
+ @VisibleForTesting
+ protected Thread createThread(Runnable runnable) {
+ Thread thread = new Thread(runnable);
+ thread.setName("JFR-Dump-Cleanup");
+ thread.setDaemon(true);
+ return thread;
}
- private static void cleanUpDumps(Path directory, int maxDumps) {
+ private static void cleanUpDumps(Path directory, int maxDumps, String
recordingName) {
if (maxDumps < 0) {
LOGGER.debug("maxDumps is negative, no cleanup will be performed");
return;
}
- File[] files = directory.toFile().listFiles();
+ File[] files = directory.toFile()
+ .listFiles((dir, name) -> name.startsWith("recording-" +
recordingName) && name.endsWith(".jfr"));
Review Comment:
The file filter logic doesn't account for the missing separator in the
filename pattern. This filter won't match files created with the current
filename format and could fail to clean up old recordings properly.
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java:
##########
@@ -118,80 +123,157 @@ public class ContinuousJfrStarter {
/// A flag to track whether the JFR recording has been started.
/// This is specially useful for testing and quickstarts, where servers,
brokers and other components are executed
/// in the same JVM.
- private static boolean _started = false;
+ public static final ContinuousJfrStarter INSTANCE = new
ContinuousJfrStarter();
+ @GuardedBy("this")
+ private boolean _running = false;
+ @GuardedBy("this")
+ private Map<String, Object> _currentConfig;
+ @GuardedBy("this")
+ private Recording _recording;
+ @GuardedBy("this")
+ private Thread _cleanupThread;
- private ContinuousJfrStarter() {
+ @VisibleForTesting
+ protected ContinuousJfrStarter() {
}
- public synchronized static void init(PinotConfiguration config) {
+ @Override
+ public void onChange(Set<String> changedConfigs, Map<String, String>
clusterConfigs) {
+ boolean jfrChanged = changedConfigs.stream()
+ .anyMatch(changedConfig ->
changedConfig.startsWith(CommonConstants.JFR));
+ if (!jfrChanged && _currentConfig != null) {
+ LOGGER.debug("ChangedConfigs: {} does not contain any JFR config.
Skipping updates", changedConfigs);
+ return;
+ }
+ PinotConfiguration config = new PinotConfiguration(clusterConfigs);
PinotConfiguration subset = config.subset(CommonConstants.JFR);
+ synchronized (this) {
+ Map<String, Object> newSubsetMap = subset.toMap();
+
+ if (_currentConfig != null && _currentConfig.equals(newSubsetMap)) {
+ // No change
+ LOGGER.debug("JFR config change detected, but no actual change in
config");
+ return;
+ }
+
+ stopRecording();
+ _currentConfig = newSubsetMap;
+ startRecording(subset);
+ }
+ }
+
+ public boolean isRunning() {
+ return _running;
+ }
+
+ private void stopRecording() {
+ if (!_running) {
+ return;
+ }
+ assert _recording != null;
Review Comment:
Using assert for runtime validation in production code is not recommended as
assertions can be disabled with -da flag. Consider using a proper null check
with exception throwing or logging instead.
```suggestion
if (_recording == null) {
LOGGER.error("Attempted to stop recording, but _recording is null.
This indicates an internal error.");
return;
}
```
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/ContinuousJfrStarter.java:
##########
@@ -118,80 +123,157 @@ public class ContinuousJfrStarter {
/// A flag to track whether the JFR recording has been started.
/// This is specially useful for testing and quickstarts, where servers,
brokers and other components are executed
/// in the same JVM.
- private static boolean _started = false;
+ public static final ContinuousJfrStarter INSTANCE = new
ContinuousJfrStarter();
+ @GuardedBy("this")
+ private boolean _running = false;
+ @GuardedBy("this")
+ private Map<String, Object> _currentConfig;
+ @GuardedBy("this")
+ private Recording _recording;
+ @GuardedBy("this")
+ private Thread _cleanupThread;
- private ContinuousJfrStarter() {
+ @VisibleForTesting
+ protected ContinuousJfrStarter() {
}
- public synchronized static void init(PinotConfiguration config) {
+ @Override
+ public void onChange(Set<String> changedConfigs, Map<String, String>
clusterConfigs) {
+ boolean jfrChanged = changedConfigs.stream()
+ .anyMatch(changedConfig ->
changedConfig.startsWith(CommonConstants.JFR));
+ if (!jfrChanged && _currentConfig != null) {
+ LOGGER.debug("ChangedConfigs: {} does not contain any JFR config.
Skipping updates", changedConfigs);
+ return;
+ }
+ PinotConfiguration config = new PinotConfiguration(clusterConfigs);
PinotConfiguration subset = config.subset(CommonConstants.JFR);
+ synchronized (this) {
+ Map<String, Object> newSubsetMap = subset.toMap();
+
+ if (_currentConfig != null && _currentConfig.equals(newSubsetMap)) {
+ // No change
+ LOGGER.debug("JFR config change detected, but no actual change in
config");
+ return;
+ }
+
+ stopRecording();
+ _currentConfig = newSubsetMap;
+ startRecording(subset);
+ }
+ }
+
+ public boolean isRunning() {
+ return _running;
+ }
+
+ 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;
+ }
+
+ LOGGER.info("Stopped continuous JFR recording {}", _recording.getName());
+ _recording = null;
+ _running = false;
+ }
+
+ private void startRecording(PinotConfiguration subset) {
if (!subset.getProperty(ENABLED, DEFAULT_ENABLED)) {
+ LOGGER.info("Continuous JFR recording is disabled");
return;
}
- if (_started) {
+ if (_running) {
return;
}
+ _recording = createRecording(subset);
+ _recording.setName(subset.getProperty(NAME, DEFAULT_NAME));
+
+ _recording.setDumpOnExit(subset.getProperty(DUMP_ON_EXIT,
DEFAULT_DUMP_ON_EXIT));
+
+ prepareFileDumps(subset);
+
+ try {
+ boolean toDisk = subset.getProperty(TO_DISK, DEFAULT_TO_DISK);
+ if (toDisk) {
+ _recording.setToDisk(true);
+ _recording.setMaxSize(subset.getProperty(MAX_SIZE, DEFAULT_MAX_SIZE));
+ _recording.setMaxAge(Duration.parse(subset.getProperty(MAX_AGE,
DEFAULT_MAX_AGE).toUpperCase(Locale.ENGLISH)));
+ }
+ } catch (DateTimeParseException e) {
+ throw new RuntimeException("Failed to parse duration", e);
+ }
+ _recording.start();
+ LOGGER.info("Started continuous JFR recording {} with configuration: {}",
_recording.getName(), subset);
+ _running = true;
+ }
+
+ private void prepareFileDumps(PinotConfiguration subset) {
+ try {
+ Path directory = Path.of(subset.getProperty(DIRECTORY,
Paths.get(".").toString()));
+ if (!directory.toFile().canWrite()) {
+ throw new RuntimeException("Cannot write: " + directory);
+ }
+
+ String timestamp = ZonedDateTime.ofInstant(Instant.now(), ZoneOffset.UTC)
+ .format(DateTimeFormatter.ofPattern("yyyy-MM-dd_HH-mm-ss"));
+ String filename = "recording-" + _recording.getName() + timestamp +
".jfr";
Review Comment:
Missing separator between recording name and timestamp in filename. This
could result in filenames like 'recording-MyRecording2024-01-01_12-00-00.jfr'
instead of the intended 'recording-MyRecording-2024-01-01_12-00-00.jfr'.
```suggestion
String filename = "recording-" + _recording.getName() + "-" +
timestamp + ".jfr";
```
--
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]
