[GitHub] [beam] lukecwik commented on a diff in pull request #17151: [BEAM-14144] Record JFR profiles when GC thrashing is detected

[GitBox <gi...@apache.org>]

lukecwik commented on code in PR #17151:
URL: https://github.com/apache/beam/pull/17151#discussion_r845292705


##########
runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowRunner.java:
##########
@@ -378,6 +378,13 @@ public static DataflowRunner fromOptions(PipelineOptions options) {
               + "' invalid. Please make sure the value is non-negative.");
     }
 
+    // Verify that if recordJfrOnGcThrashing is set, the pipeline is at least on java 11
+    if (dataflowOptions.getRecordJfrOnGcThrashing()
+        && Environments.getJavaVersion() == Environments.JavaVersion.java8) {
+      throw new IllegalArgumentException(
+          "recordJfrOnGcThrashing is only supported on java 9 and up.");

Review Comment:
   ```suggestion
             "recordJfrOnGcThrashing is only supported on Java 9 and up.");
   ```



##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/MemoryMonitor.java:
##########
@@ -198,20 +227,43 @@ public long totalGCTimeMilliseconds() {
 
   private final File localDumpFolder;
 
+  private final String workerId;
+
+  private final @Nullable JfrInterop jfrInterop;
+
+  private final Clock clock;
+
   public static MemoryMonitor fromOptions(PipelineOptions options) {
     DataflowPipelineDebugOptions debugOptions = options.as(DataflowPipelineDebugOptions.class);
+    DataflowWorkerHarnessOptions workerHarnessOptions =
+        options.as(DataflowWorkerHarnessOptions.class);
     String uploadToGCSPath = debugOptions.getSaveHeapDumpsToGcsPath();
+    String workerId = workerHarnessOptions.getWorkerId();
     boolean canDumpHeap = uploadToGCSPath != null || debugOptions.getDumpHeapOnOOM();
     double gcThrashingPercentagePerPeriod = debugOptions.getGCThrashingPercentagePerPeriod();
 
+    Duration jfrProfileDuration;
+    if (uploadToGCSPath != null && debugOptions.getRecordJfrOnGcThrashing()) {
+      if (Environments.getJavaVersion() == Environments.JavaVersion.java8) {
+        throw new IllegalArgumentException(
+            "recordJfrOnGcThrashing is only supported on java 9 and up.");

Review Comment:
   ```suggestion
               "recordJfrOnGcThrashing is only supported on Java 9 and up.");
   ```



##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/MemoryMonitor.java:
##########
@@ -515,6 +562,10 @@ public void run() {
         if (isThrashing.get()) {
           currentThrashingCount++;
 
+          if (currentThrashingCount == 1) {
+            runJfrProfileOnHeapThrashing();
+          }
+

Review Comment:
   I thought it was wrong but I misaligned the spacing.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org