No error/warning on failed snapshot deletion #10676 (#10805)

No error/warning on failed snapshot deletion #10676

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

@@ -13,9 +13,22 @@
 public class DeleteSnapshotsResult
     extends AbstractImmutableEntitySet<String>
 {
+    private final Set<String> failedSnapshots;
+
     private DeleteSnapshotsResult( final Builder builder )
     {
         super( ImmutableSet.copyOf( builder.snapshotNames ) );
+        this.failedSnapshots = ImmutableSet.copyOf( builder.failedSnapshots );
+    }
+
+    public Set<String> getDeletedSnapshots()
+    {
+        return this.getSet();
+    }
+
+    public Set<String> getFailedSnapshots()
+    {
+        return failedSnapshots;
     }
 
     public static Builder create()
@@ -27,19 +40,28 @@ public static class Builder
     {
         private final Set<String> snapshotNames = new HashSet<>();
 
+        private final Set<String> failedSnapshots = new HashSet<>();
+
 
         public Builder add( final String snapshotName )
         {
             this.snapshotNames.add( snapshotName );
             return this;
         }
 
+        @Deprecated
         public Builder addAll( final Collection<String> snapshotNames )
         {
             this.snapshotNames.addAll( snapshotNames );
             return this;
         }
 
+        public Builder addFailed( final String snapshotName )
+        {
+            this.failedSnapshots.add( snapshotName );
+            return this;
+        }
+
         public DeleteSnapshotsResult build()
         {
             return new DeleteSnapshotsResult( this );

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;
@@ -316,46 +315,54 @@ private DeleteSnapshotsResult doDelete( final DeleteSnapshotParams params )
 
         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 DeleteSnapshotsResult.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.add( 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 DeleteSnapshotsResult.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.add( 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,8 +5,6 @@
 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;
@@ -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,13 @@ 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() );
+        final DeleteSnapshotsResult deleteSnapshotsResult =
+            snapshotService.delete( DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() );
 
-            deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() );
+        deleteSnapshotsResult.getDeletedSnapshots().forEach( snapshot -> builder.processed() );
+        deleteSnapshotsResult.getFailedSnapshots().forEach( snapshot -> builder.failed() );
 
-            return builder.build();
-        }
-        catch ( Exception e )
-        {
-            LOG.error( "Failed to vacuum snapshots", e );
-            return builder.failed().build();
-        }
+        return builder.build();
     }
 
     @Override

modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImplTest.java

@@ -0,0 +1,120 @@
+package com.enonic.xp.repo.impl.elasticsearch.snapshot;
+
+import java.nio.file.Path;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.List;
+
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.action.ActionFuture;
+import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesRequest;
+import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse;
+import org.elasticsearch.action.admin.cluster.snapshots.delete.DeleteSnapshotRequest;
+import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest;
+import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse;
+import org.elasticsearch.client.AdminClient;
+import org.elasticsearch.client.Client;
+import org.elasticsearch.client.ClusterAdminClient;
+import org.elasticsearch.cluster.metadata.RepositoryMetaData;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.snapshots.SnapshotInfo;
+import org.elasticsearch.snapshots.SnapshotState;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import com.enonic.xp.event.EventPublisher;
+import com.enonic.xp.node.DeleteSnapshotParams;
+import com.enonic.xp.node.DeleteSnapshotsResult;
+import com.enonic.xp.repo.impl.config.RepoConfiguration;
+import com.enonic.xp.repo.impl.index.IndexServiceInternal;
+import com.enonic.xp.repo.impl.repository.RepositoryEntryService;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class SnapshotServiceImplTest
+{
+    private ClusterAdminClient clusterAdminClient;
+
+    private SnapshotServiceImpl instance;
+
+    @BeforeEach
+    void setUp()
+    {
+        final Client client = mock( Client.class );
+        clusterAdminClient = mock( ClusterAdminClient.class );
+
+        when( client.admin() ).thenReturn( mock( AdminClient.class ) );
+        when( client.admin().cluster() ).thenReturn( clusterAdminClient );
+
+        final RepoConfiguration configuration = mock( RepoConfiguration.class );
+        when( configuration.getSnapshotsDir() ).thenReturn( Path.of( "tmp", "data" ) );
+
+        final RepositoryEntryService repositoryEntryService = mock( RepositoryEntryService.class );
+        final EventPublisher eventPublisher = mock( EventPublisher.class );
+        final IndexServiceInternal indexServiceInternal = mock( IndexServiceInternal.class );
+
+        instance = new SnapshotServiceImpl( client, configuration, repositoryEntryService, eventPublisher, indexServiceInternal );
+    }
+
+    @Test
+    void testDelete()
+    {
+        final RepositoryMetaData repositoryMetaData = new RepositoryMetaData( "enonic-xp-snapshot-repo", "fs", Settings.settingsBuilder()
+            .put( "compress", true )
+            .put( "location", Path.of( "tmp", "data" ).toString() )
+            .build() );
+
+        final GetRepositoriesResponse getRepositoriesResponse = mock( GetRepositoriesResponse.class );
+        when( getRepositoriesResponse.repositories() ).thenReturn( List.of( repositoryMetaData ) );
+
+        final ActionFuture<GetRepositoriesResponse> getRepositoryActionFuture = mock( ActionFuture.class );
+        when( getRepositoryActionFuture.actionGet() ).thenReturn( getRepositoriesResponse );
+
+        when( clusterAdminClient.getRepositories( any( GetRepositoriesRequest.class ) ) ).thenReturn( getRepositoryActionFuture );
+
+        final Instant now = Instant.now();
+
+        final SnapshotInfo snapshot3 = mockSnapshot( "snapshot3", SnapshotState.SUCCESS, now.minus( 3, ChronoUnit.HOURS ) );
+        final SnapshotInfo snapshot4 = mockSnapshot( "snapshot4", SnapshotState.PARTIAL, now.minus( 4, ChronoUnit.HOURS ) );
+        final SnapshotInfo snapshot5 = mockSnapshot( "snapshot5", SnapshotState.SUCCESS, now.minus( 1, ChronoUnit.HOURS ) );
+
+        final GetSnapshotsResponse getSnapshotsResponse = mock( GetSnapshotsResponse.class );
+        when( getSnapshotsResponse.getSnapshots() ).thenReturn( List.of( snapshot3, snapshot4, snapshot5 ) );
+
+        final ActionFuture<GetSnapshotsResponse> getSnapshotsActionFuture = mock( ActionFuture.class );
+        when( getSnapshotsActionFuture.actionGet() ).thenReturn( getSnapshotsResponse );
+
+        when( clusterAdminClient.getSnapshots( any( GetSnapshotsRequest.class ) ) ).thenReturn( getSnapshotsActionFuture );
+
+        when( clusterAdminClient.deleteSnapshot( any( DeleteSnapshotRequest.class ) ) ).thenReturn( mock( ActionFuture.class ) );
+        when( clusterAdminClient.deleteSnapshot(
+            argThat( request -> "snapshot2".equals( request.snapshot() ) || "snapshot4".equals( request.snapshot() ) ) ) ).thenThrow(
+            new ElasticsearchException( "Failed to delete snapshot" ) );
+
+        final DeleteSnapshotsResult result = instance.delete(
+            DeleteSnapshotParams.create().add( "snapshot1" ).add( "snapshot2" ).before( now.minus( 2, ChronoUnit.HOURS ) ).build() );
+
+        assertEquals( 2, result.getSize() );
+        assertTrue( result.getSet().contains( "snapshot1" ) );
+        assertTrue( result.getSet().contains( "snapshot3" ) );
+        assertEquals( 2, result.getFailedSnapshots().size() );
+        assertTrue( result.getFailedSnapshots().contains( "snapshot2" ) );
+        assertTrue( result.getFailedSnapshots().contains( "snapshot4" ) );
+    }
+
+    private static SnapshotInfo mockSnapshot( String name, SnapshotState state, Instant endTime )
+    {
+        final SnapshotInfo snapshot = mock( SnapshotInfo.class );
+
+        when( snapshot.state() ).thenReturn( state );
+        when( snapshot.name() ).thenReturn( name );
+        when( snapshot.endTime() ).thenReturn( endTime.toEpochMilli() );
+
+        return snapshot;
+    }
+}

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

@@ -64,23 +64,4 @@ public void processed( final long count )
 
         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 ) );
-    }
 }

modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/handler/mapping/ComponentServiceMappingHandler.java

@@ -5,12 +5,16 @@
 import org.osgi.service.component.annotations.Reference;
 
 import com.enonic.xp.content.ContentService;
+import com.enonic.xp.page.PageDescriptorService;
+import com.enonic.xp.page.PageTemplateService;
 import com.enonic.xp.portal.controller.ControllerScriptFactory;
 import com.enonic.xp.portal.filter.FilterScriptFactory;
 import com.enonic.xp.portal.handler.EndpointHandler;
 import com.enonic.xp.portal.impl.ContentResolver;
+import com.enonic.xp.portal.impl.handler.render.PageResolver;
 import com.enonic.xp.portal.impl.rendering.RendererDelegate;
 import com.enonic.xp.project.ProjectService;
+import com.enonic.xp.region.LayoutDescriptorService;
 import com.enonic.xp.resource.ResourceService;
 import com.enonic.xp.site.SiteService;
 import com.enonic.xp.web.HttpMethod;
@@ -30,13 +34,17 @@ public ComponentServiceMappingHandler( @Reference final ProjectService projectSe
                                            @Reference final ControllerScriptFactory controllerScriptFactory,
                                            @Reference final FilterScriptFactory filterScriptFactory,
                                            @Reference final RendererDelegate rendererDelegate, @Reference final SiteService siteService,
-                                           @Reference final ContentService contentService )
+                                           @Reference final ContentService contentService,
+                                           @Reference final PageTemplateService pageTemplateService,
+                                           @Reference final PageDescriptorService pageDescriptorService,
+                                           @Reference final LayoutDescriptorService layoutDescriptorService )
     {
         super( HttpMethod.standard(), "component" );
 
         this.mappingHandlerHelper =
             new MappingHandlerHelper( projectService, resourceService, controllerScriptFactory, filterScriptFactory, rendererDelegate,
-                                      new ControllerMappingsResolver( siteService ), new ContentResolver( contentService ) );
+                                      new ControllerMappingsResolver( siteService ), new ContentResolver( contentService ),
+                                      new PageResolver( pageTemplateService, pageDescriptorService, layoutDescriptorService ));
     }
 
     @Override

modules/portal/portal-impl/src/main/java/com/enonic/xp/portal/impl/handler/mapping/MappingHandler.java

@@ -5,11 +5,15 @@
 import org.osgi.service.component.annotations.Reference;
 
 import com.enonic.xp.content.ContentService;
+import com.enonic.xp.page.PageDescriptorService;
+import com.enonic.xp.page.PageTemplateService;
 import com.enonic.xp.portal.controller.ControllerScriptFactory;
 import com.enonic.xp.portal.filter.FilterScriptFactory;
 import com.enonic.xp.portal.impl.ContentResolver;
+import com.enonic.xp.portal.impl.handler.render.PageResolver;
 import com.enonic.xp.portal.impl.rendering.RendererDelegate;
 import com.enonic.xp.project.ProjectService;
+import com.enonic.xp.region.LayoutDescriptorService;
 import com.enonic.xp.resource.ResourceService;
 import com.enonic.xp.site.SiteService;
 import com.enonic.xp.web.WebRequest;
@@ -28,11 +32,15 @@ public MappingHandler( @Reference final SiteService siteService, @Reference fina
                            @Reference final ResourceService resourceService,
                            @Reference final ControllerScriptFactory controllerScriptFactory,
                            @Reference final FilterScriptFactory filterScriptFactory, @Reference final RendererDelegate rendererDelegate,
-                           @Reference final ProjectService projectService )
+                           @Reference final ProjectService projectService, @Reference final PageTemplateService pageTemplateService,
+                           @Reference final PageDescriptorService pageDescriptorService,
+                           @Reference final LayoutDescriptorService layoutDescriptorService
+                           )
     {
         this.mappingHandlerHelper =
             new MappingHandlerHelper( projectService, resourceService, controllerScriptFactory, filterScriptFactory, rendererDelegate,
-                                      new ControllerMappingsResolver( siteService ), new ContentResolver( contentService ) );
+                                      new ControllerMappingsResolver( siteService ), new ContentResolver( contentService ),
+                                      new PageResolver( pageTemplateService, pageDescriptorService, layoutDescriptorService ) );
     }
 
     @Override