No error/warning on failed snapshot deletion #10676

modules/core/core-api/src/main/java/com/enonic/xp/node/DeleteSnapshotsResult.java

@@ -9,6 +9,7 @@
 import com.enonic.xp.annotation.PublicApi;
 import com.enonic.xp.support.AbstractImmutableEntitySet;
 
+@Deprecated
 @PublicApi
 public class DeleteSnapshotsResult
     extends AbstractImmutableEntitySet<String>

modules/core/core-api/src/main/java/com/enonic/xp/node/RemoveSnapshotsResult.java

@@ -0,0 +1,60 @@
+package com.enonic.xp.node;
+
+import java.util.Set;
+
+import com.google.common.collect.ImmutableSet;
+
+import com.enonic.xp.annotation.PublicApi;
+
+@PublicApi
+public final class RemoveSnapshotsResult
+{
+    private final Set<String> snapshotNames;
+
+    private final Set<String> failedSnapshotNames;
+
+    private RemoveSnapshotsResult( final Builder builder )
+    {
+        this.snapshotNames = builder.snapshotNames.build();
+        this.failedSnapshotNames = builder.failedSnapshotNames.build();
+    }
+
+    public Set<String> getSnapshotNames()
+    {
+        return snapshotNames;
+    }
+
+    public Set<String> getFailedSnapshotNames()
+    {
+        return failedSnapshotNames;
+    }
+
+    public static Builder create()
+    {
+        return new Builder();
+    }
+
+    public static final class Builder
+    {
+        private final ImmutableSet.Builder<String> snapshotNames = ImmutableSet.builder();
+
+        private final ImmutableSet.Builder<String> failedSnapshotNames = ImmutableSet.builder();
+
+        public Builder addProcessed( final String snapshotName )
+        {
+            this.snapshotNames.add( snapshotName );
+            return this;
+        }
+
+        public Builder addFailed( final String snapshotName )
+        {
+            this.failedSnapshotNames.add( snapshotName );
+            return this;
+        }
+
+        public RemoveSnapshotsResult build()
+        {
+            return new RemoveSnapshotsResult( this );
+        }
+    }
+}

modules/core/core-api/src/main/java/com/enonic/xp/snapshot/SnapshotService.java

@@ -2,6 +2,7 @@
 
 import com.enonic.xp.node.DeleteSnapshotParams;
 import com.enonic.xp.node.DeleteSnapshotsResult;
+import com.enonic.xp.node.RemoveSnapshotsResult;
 import com.enonic.xp.node.RestoreParams;
 import com.enonic.xp.node.RestoreResult;
 import com.enonic.xp.node.SnapshotParams;
@@ -14,7 +15,10 @@ public interface SnapshotService
 
     RestoreResult restore( RestoreParams restoreParams );
 
+    @Deprecated
     DeleteSnapshotsResult delete( DeleteSnapshotParams params );
 
+    RemoveSnapshotsResult remove( DeleteSnapshotParams params );
+
     SnapshotResults list();
 }

modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImpl.java

@@ -2,7 +2,6 @@
 
 import java.nio.file.Path;
 import java.time.Instant;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
@@ -33,6 +32,7 @@
 import com.enonic.xp.event.EventPublisher;
 import com.enonic.xp.node.DeleteSnapshotParams;
 import com.enonic.xp.node.DeleteSnapshotsResult;
+import com.enonic.xp.node.RemoveSnapshotsResult;
 import com.enonic.xp.node.RestoreParams;
 import com.enonic.xp.node.RestoreResult;
 import com.enonic.xp.node.SnapshotParams;
@@ -197,8 +197,18 @@ private SnapshotResults doListSnapshots()
         return SnapshotResultsFactory.create( getSnapshotsResponse );
     }
 
+    @Deprecated
     @Override
     public DeleteSnapshotsResult delete( final DeleteSnapshotParams params )
+    {
+        return NodeHelper.runAsAdmin( () -> {
+            final RemoveSnapshotsResult result = doDelete( params );
+            return DeleteSnapshotsResult.create().addAll( result.getSnapshotNames() ).build();
+        } );
+    }
+
+    @Override
+    public RemoveSnapshotsResult remove( final DeleteSnapshotParams params )
     {
         return NodeHelper.runAsAdmin( () -> doDelete( params ) );
     }
@@ -310,52 +320,60 @@ private SnapshotInfo getSnapshot( final String snapshotName )
         }
     }
 
-    private DeleteSnapshotsResult doDelete( final DeleteSnapshotParams params )
+    private RemoveSnapshotsResult doDelete( final DeleteSnapshotParams params )
     {
-        final DeleteSnapshotsResult.Builder builder = DeleteSnapshotsResult.create();
+        final RemoveSnapshotsResult.Builder builder = RemoveSnapshotsResult.create();
 
         if ( !params.getSnapshotNames().isEmpty() )
         {
-            builder.addAll( deleteByName( params.getSnapshotNames() ) );
+            deleteByName( builder, params.getSnapshotNames() );
         }
 
         if ( params.getBefore() != null )
         {
-            builder.addAll( deleteByBefore( params.getBefore() ) );
+            deleteByBefore( builder, params.getBefore() );
         }
 
         return builder.build();
     }
 
-    private Set<String> deleteByBefore( final Instant before )
+    private void deleteByBefore( final RemoveSnapshotsResult.Builder builder, final Instant before )
     {
-        final Set<String> deleted = new HashSet<>();
-
         final SnapshotResults snapshotResults = doListSnapshots();
 
         for ( final SnapshotResult snapshotResult : snapshotResults )
         {
             if ( snapshotResult.getTimestamp().isBefore( before ) )
             {
-                doDeleteSnapshot( snapshotResult.getName() );
-                deleted.add( snapshotResult.getName() );
+                try
+                {
+                    doDeleteSnapshot( snapshotResult.getName() );
+                    builder.addProcessed( snapshotResult.getName() );
+                }
+                catch ( Exception e )
+                {
+                    LOG.error( "Snapshot delete failed: {}", snapshotResult.getName(), e );
+                    builder.addFailed( snapshotResult.getName() );
+                }
             }
         }
-
-        return deleted;
     }
 
-    private Set<String> deleteByName( final Set<String> snapshotNames )
+    private void deleteByName( final RemoveSnapshotsResult.Builder builder, final Set<String> snapshotNames )
     {
-        final Set<String> deletedNames = new HashSet<>();
-
         for ( final String name : snapshotNames )
         {
-            doDeleteSnapshot( name );
-            deletedNames.add( name );
+            try
+            {
+                doDeleteSnapshot( name );
+                builder.addProcessed( name );
+            }
+            catch ( Exception e )
+            {
+                LOG.error( "Snapshot delete failed: {}", name, e );
+                builder.addFailed( name );
+            }
         }
-
-        return deletedNames;
     }
 
     private void doDeleteSnapshot( final String snapshotName )

modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTask.java

@@ -5,11 +5,9 @@
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.Reference;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import com.enonic.xp.node.DeleteSnapshotParams;
-import com.enonic.xp.node.DeleteSnapshotsResult;
+import com.enonic.xp.node.RemoveSnapshotsResult;
 import com.enonic.xp.repo.impl.vacuum.VacuumTask;
 import com.enonic.xp.repo.impl.vacuum.VacuumTaskParams;
 import com.enonic.xp.snapshot.SnapshotService;
@@ -19,8 +17,6 @@
 public class SnapshotsVacuumTask
     implements VacuumTask
 {
-    private static final Logger LOG = LoggerFactory.getLogger( SnapshotsVacuumTask.class );
-
     private static final int ORDER = 400;
 
     private static final String NAME = "SnapshotsVacuumTask";
@@ -42,20 +38,14 @@ public VacuumTaskResult execute( final VacuumTaskParams params )
         }
 
         final VacuumTaskResult.Builder builder = VacuumTaskResult.create().taskName( NAME );
-        try
-        {
-            final DeleteSnapshotsResult deleteSnapshotsResult = snapshotService.delete(
-                DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() );
 
-            deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() );
+        final RemoveSnapshotsResult deleteSnapshotsResult =
+            snapshotService.remove( DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() );
 
-            return builder.build();
-        }
-        catch ( Exception e )
-        {
-            LOG.error( "Failed to vacuum snapshots", e );
-            return builder.failed().build();
-        }
+        deleteSnapshotsResult.getSnapshotNames().forEach( snapshot -> builder.processed() );
+        deleteSnapshotsResult.getFailedSnapshotNames().forEach( snapshot -> builder.failed() );
+
+        return builder.build();
     }
 
     @Override

modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTaskTest.java

@@ -3,7 +3,7 @@
 import org.junit.jupiter.api.Test;
 
 import com.enonic.xp.node.DeleteSnapshotParams;
-import com.enonic.xp.node.DeleteSnapshotsResult;
+import com.enonic.xp.node.RemoveSnapshotsResult;
 import com.enonic.xp.repo.impl.vacuum.VacuumTaskParams;
 import com.enonic.xp.snapshot.SnapshotService;
 import com.enonic.xp.vacuum.VacuumListener;
@@ -22,8 +22,8 @@ public class SnapshotsVacuumTaskTest
     void test()
     {
         SnapshotService snapshotService = mock( SnapshotService.class );
-        when( snapshotService.delete( any( DeleteSnapshotParams.class ) ) ).thenReturn(
-            DeleteSnapshotsResult.create().add( "snapshot" ).build() );
+        when( snapshotService.remove( any( DeleteSnapshotParams.class ) ) ).thenReturn(
+            RemoveSnapshotsResult.create().addProcessed( "snapshot" ).build() );
 
         SnapshotsVacuumTask instance = new SnapshotsVacuumTask( snapshotService );
 
@@ -62,25 +62,6 @@ public void processed( final long count )
         assertEquals( 0, result.getDeleted() );
         assertEquals( 0, result.getFailed() );
 
-        verify( snapshotService, times( 1 ) ).delete( any( DeleteSnapshotParams.class ) );
-    }
-
-    @Test
-    void testFailed()
-    {
-        SnapshotService snapshotService = mock( SnapshotService.class );
-        when( snapshotService.delete( any( DeleteSnapshotParams.class ) ) ).thenThrow( new RuntimeException() );
-
-        SnapshotsVacuumTask instance = new SnapshotsVacuumTask( snapshotService );
-
-        VacuumTaskResult result = instance.execute( VacuumTaskParams.create().ageThreshold( 60 * 1000 ).build() );
-
-        assertEquals( "SnapshotsVacuumTask", result.getTaskName() );
-        assertEquals( 0, result.getProcessed() );
-        assertEquals( 0, result.getInUse() );
-        assertEquals( 0, result.getDeleted() );
-        assertEquals( 1, result.getFailed() );
-
-        verify( snapshotService, times( 1 ) ).delete( any( DeleteSnapshotParams.class ) );
+        verify( snapshotService, times( 1 ) ).remove( any( DeleteSnapshotParams.class ) );
     }
 }

modules/itest/itest-core/src/test/java/com/enonic/xp/core/snapshot/SnapshotServiceImplTest.java

@@ -252,7 +252,7 @@ private void doRestoreInvalidSnapshot()
 
         this.snapshotService.snapshot( SnapshotParams.create().snapshotName( "my-snapshot" ).build() );
 
-        this.snapshotService.delete( DeleteSnapshotParams.create().add( "my-snapshot" ).build() );
+        this.snapshotService.remove( DeleteSnapshotParams.create().add( "my-snapshot" ).build() );
 
         this.repositoryService.deleteRepository( DeleteRepositoryParams.from( newRepoId ) );