refactor, cleanup, fix broken "restart if already exists"

cryostatio · Aug 29, 2023 · a62f744 · a62f744
1 parent e566c57
commit a62f744
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 47 deletions.
diff --git a/src/main/java/io/cryostat/recordings/ActiveRecording.java b/src/main/java/io/cryostat/recordings/ActiveRecording.java
@@ -18,6 +18,7 @@
 import java.io.IOException;
 import java.net.URI;
 import java.util.Objects;
+import java.util.Optional;
 
 import org.openjdk.jmc.common.unit.UnitLookup;
 import org.openjdk.jmc.rjmx.ServiceNotAvailableException;
@@ -48,6 +49,7 @@
 import jakarta.persistence.PreUpdate;
 import jakarta.persistence.Table;
 import jakarta.persistence.UniqueConstraint;
+import jakarta.transaction.Transactional;
 import jdk.jfr.RecordingState;
 import org.hibernate.annotations.JdbcTypeCode;
 import org.hibernate.type.SqlTypes;
@@ -138,19 +140,25 @@ public static ActiveRecording getByName(String name) {
         return find("name", name).singleResult();
     }
 
-    // Default implementation of delete is bulk so does not trigger lifecycle events
-    public static boolean deleteByName(String name) {
-        return delete("name", name) > 0;
-    }
-
+    @Transactional
     public static boolean deleteFromTarget(Target target, String recordingName) {
-        ActiveRecording recordingToDelete =
-                find("target = ?1 and name = ?2", target, recordingName).firstResult();
-        if (recordingToDelete != null) {
-            recordingToDelete.delete();
-            return true;
+        Optional<ActiveRecording> recording =
+                target.activeRecordings.stream()
+                        .filter(r -> r.name.equals(recordingName))
+                        .findFirst();
+        boolean found = recording.isPresent();
+        if (found) {
+            Logger.getLogger(ActiveRecording.class)
+                    .infov("Found and deleting match: {0} / {1}", target.alias, recording.get());
+            recording.get().delete();
+            getEntityManager().flush();
+        } else {
+            Logger.getLogger(ActiveRecording.class)
+                    .infov(
+                            "No match found for recording {0} in target {1}",
+                            recordingName, target.alias);
         }
-        return false; // Recording not found or already deleted.
+        return found;
     }
 
     @ApplicationScoped

diff --git a/src/main/java/io/cryostat/recordings/RecordingHelper.java b/src/main/java/io/cryostat/recordings/RecordingHelper.java
@@ -139,28 +139,6 @@ public class RecordingHelper {
     @ConfigProperty(name = ConfigProperties.AWS_OBJECT_EXPIRATION_LABELS)
     String objectExpirationLabel;
 
-    boolean shouldRestartRecording(
-            RecordingReplace replace, RecordingState state, String recordingName)
-            throws BadRequestException {
-        switch (replace) {
-            case ALWAYS:
-                return true;
-            case NEVER:
-                return false;
-            case STOPPED:
-                if (state == RecordingState.RUNNING) {
-                    throw new BadRequestException(
-                            String.format(
-                                    "replace=='STOPPED' but recording with name \"%s\" is already"
-                                            + " running",
-                                    recordingName));
-                }
-                return state == RecordingState.STOPPED;
-            default:
-                return true;
-        }
-    }
-
     public ActiveRecording startRecording(
             Target target,
             IConstrainedMap<String> recordingOptions,
@@ -174,20 +152,28 @@ public ActiveRecording startRecording(
         String recordingName = (String) recordingOptions.get(RecordingOptionsBuilder.KEY_NAME);
         TemplateType preferredTemplateType =
                 getPreferredTemplateType(connection, templateName, templateType);
-        Optional<IRecordingDescriptor> previous = getDescriptorByName(connection, recordingName);
-        if (previous.isPresent()) {
-            RecordingState previousState = mapState(previous.get());
-            boolean restart = shouldRestartRecording(replace, previousState, recordingName);
-            if (!restart) {
-                throw new BadRequestException(
-                        String.format("Recording with name \"%s\" already exists", recordingName));
-            }
-            if (!ActiveRecording.deleteFromTarget(target, recordingName)) {
-                logger.warnf(
-                        "Could not delete recording %s from target %s",
-                        recordingName, target.alias);
-            }
-        }
+        getDescriptorByName(connection, recordingName)
+                .ifPresent(
+                        previous -> {
+                            RecordingState previousState = mapState(previous);
+                            boolean restart =
+                                    shouldRestartRecording(replace, previousState, recordingName);
+                            logger.infov(
+                                    "Found existing {1} recording named {0}. Replacement requested:"
+                                            + " {3}. Restarting? {2}",
+                                    recordingName, previousState, restart, replace);
+                            if (!restart) {
+                                throw new BadRequestException(
+                                        String.format(
+                                                "Recording with name \"%s\" already exists",
+                                                recordingName));
+                            }
+                            if (!ActiveRecording.deleteFromTarget(target, recordingName)) {
+                                logger.warnf(
+                                        "Could not delete recording %s from target %s",
+                                        recordingName, target.alias);
+                            }
+                        });
 
         IRecordingDescriptor desc =
                 connection
@@ -210,6 +196,28 @@ public ActiveRecording startRecording(
         return recording;
     }
 
+    private boolean shouldRestartRecording(
+            RecordingReplace replace, RecordingState state, String recordingName)
+            throws BadRequestException {
+        switch (replace) {
+            case ALWAYS:
+                return true;
+            case NEVER:
+                return false;
+            case STOPPED:
+                if (state == RecordingState.RUNNING) {
+                    throw new BadRequestException(
+                            String.format(
+                                    "replace=='STOPPED' but recording with name \"%s\" is already"
+                                            + " running",
+                                    recordingName));
+                }
+                return state == RecordingState.STOPPED;
+            default:
+                return true;
+        }
+    }
+
     public LinkedRecordingDescriptor toExternalForm(ActiveRecording recording) {
         return new LinkedRecordingDescriptor(
                 recording.remoteId,