Issue 18: Housekeeping shouldn't fail when attempting to cleanup a path that is already deleted

CHANGELOG.md

@@ -1,6 +1,8 @@
 ## TBD
 ### Changed
 * Default Housekeeping configuration is now provided via `@Bean` `housekeepingEnvironment` only if the bean is not already provided.
+### Fixed
+* Fixed issue where Housekeeping was failing when database contained a path which no longer exists. See [#18](https://github.com/HotelsDotCom/housekeeping/issues/18).
 
 ## [1.0.2] 2018-02-15
 ### Added

src/main/java/com/hotels/housekeeping/HousekeepingException.java

@@ -0,0 +1,33 @@
+/**
+ * Copyright (C) 2016-2018 Expedia Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.hotels.housekeeping;
+
+public class HousekeepingException extends RuntimeException {
+  private static final long serialVersionUID = 1L;
+
+  public HousekeepingException(Exception e) {
+    super(e);
+  }
+
+  public HousekeepingException(String message) {
+    super(message);
+  }
+
+  public HousekeepingException(String message, Throwable cause) {
+    super(message, cause);
+  }
+
+}

src/main/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingService.java

@@ -29,6 +29,7 @@
 import org.slf4j.LoggerFactory;
 import org.springframework.orm.ObjectOptimisticLockingFailureException;
 
+import com.hotels.housekeeping.HousekeepingException;
 import com.hotels.housekeeping.model.LegacyReplicaPath;
 import com.hotels.housekeeping.repository.LegacyReplicaPathRepository;
 import com.hotels.housekeeping.service.HousekeepingService;
@@ -47,39 +48,62 @@ public FileSystemHousekeepingService(LegacyReplicaPathRepository legacyReplicaPa
     LOG.warn("{}.fixIncompleteRecord(LegacyReplicaPath) should be removed in future.", getClass());
   }
 
+  private FileSystem fileSystemForPath(Path path) {
+    try {
+      return path.getFileSystem(conf);
+    } catch (IOException e) {
+      throw new HousekeepingException(e);
+    }
+  }
+
+  private void housekeepPath(LegacyReplicaPath cleanUpPath) {
+    final Path path = new Path(cleanUpPath.getPath());
+    final Path rootPath;
+    final FileSystem fs;
+    try {
+      fs = fileSystemForPath(new Path(cleanUpPath.getPath()));
+      LOG.info("Attempting to delete path '{}' from file system", cleanUpPath);
+      if (fs.exists(path)) {
+        fs.delete(path, true);
+        LOG.info("Path '{}' has been deleted from file system", cleanUpPath);
+      } else {
+        LOG.warn("Path '{}' does not exist.", cleanUpPath);
+      }
+      rootPath = deleteParents(fs, path, cleanUpPath.getPathEventId());
+    } catch (Exception e) {
+      LOG.warn("Unable to delete path '{}' from file system. Will try next time. {}", cleanUpPath, e.getMessage());
+      return;
+    }
+
+    try {
+      if (oneOfMySiblingsWillTakeCareOfMyAncestors(path, rootPath, fs) || thereIsNothingMoreToDelete(fs, rootPath)) {
+        // BEWARE the eventual consistency of your blobstore!
+        try {
+          LOG.info("Deleting path '{}' from housekeeping database", cleanUpPath);
+          legacyReplicaPathRepository.delete(cleanUpPath);
+        } catch (ObjectOptimisticLockingFailureException e) {
+          LOG.debug(
+              "Failed to delete path '{}': probably already cleaned up by process running at same time. Ok to ignore. {}",
+              cleanUpPath, e.getMessage());
+        }
+      }
+    } catch (Exception e) {
+      LOG.warn("Path '{}' was not deleted from the housekeeping database. {}", cleanUpPath, e.getMessage());
+    }
+  }
+
   @Override
   public void cleanUp(Instant referenceTime) {
     try {
       List<LegacyReplicaPath> pathsToDelete = legacyReplicaPathRepository
           .findByCreationTimestampLessThanEqual(referenceTime.getMillis());
       for (LegacyReplicaPath cleanUpPath : pathsToDelete) {
         cleanUpPath = fixIncompleteRecord(cleanUpPath);
-        LOG.debug("Deleting path '{}' from file system", cleanUpPath);
-        Path path = new Path(cleanUpPath.getPath());
-        FileSystem fs = path.getFileSystem(conf);
-        Path rootPath;
-        try {
-          fs.delete(path, true);
-          rootPath = deleteParents(fs, path, cleanUpPath.getPathEventId());
-          LOG.debug("Path '{}' has been deleted from file system", cleanUpPath);
-        } catch (Exception e) {
-          LOG.warn("Unable to delete path '{}' from file system. Will try next time", cleanUpPath, e);
-          continue;
-        }
-        if (oneOfMySiblingsWillTakeCareOfMyAncestors(path, rootPath, fs) || thereIsNothingMoreToDelete(fs, rootPath)) {
-          // BEWARE the eventual consistency of your blobstore!
-          try {
-            LOG.debug("Deleting path '{}' from database", cleanUpPath);
-            legacyReplicaPathRepository.delete(cleanUpPath);
-          } catch (ObjectOptimisticLockingFailureException e) {
-            LOG.debug(
-                "Failed to delete path '{}': probably already cleaned up by process running at same time. Ok to ignore",
-                cleanUpPath);
-          }
-        }
+        housekeepPath(cleanUpPath);
       }
     } catch (Exception e) {
-      throw new RuntimeException(format("Unable to execute housekeeping at instant %d", referenceTime.getMillis()), e);
+      throw new HousekeepingException(format("Unable to execute housekeeping at instant %d", referenceTime.getMillis()),
+          e);
     }
   }
 
@@ -110,7 +134,7 @@ private Path deleteParents(FileSystem fs, Path path, String eventId) throws IOEx
     }
     Path parent = path.getParent();
     if (fs.exists(parent) && isEmpty(fs, parent)) {
-      LOG.debug("Deleting parent path '{}'", parent);
+      LOG.info("Deleting parent path '{}'", parent);
       fs.delete(parent, false);
     }
     return deleteParents(fs, parent, eventId);
@@ -125,7 +149,7 @@ public void scheduleForHousekeeping(LegacyReplicaPath cleanUpPath) {
     try {
       legacyReplicaPathRepository.save(cleanUpPath);
     } catch (Exception e) {
-      throw new RuntimeException(format("Unable to schedule path %s for deletion", cleanUpPath.getPath()), e);
+      throw new HousekeepingException(format("Unable to schedule path %s for deletion", cleanUpPath.getPath()), e);
     }
   }
 

src/test/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingServiceTest.java

@@ -20,14 +20,18 @@
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doCallRealMethod;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.powermock.api.mockito.PowerMockito.mockStatic;
+import static org.powermock.api.mockito.PowerMockito.verifyPrivate;
 
 import java.io.File;
 import java.io.IOException;
@@ -49,6 +53,7 @@
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.Spy;
+import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 import org.springframework.orm.ObjectOptimisticLockingFailureException;
@@ -58,7 +63,7 @@
 import com.hotels.housekeeping.repository.LegacyReplicaPathRepository;
 
 @RunWith(PowerMockRunner.class)
-@PrepareForTest(FileSystem.class)
+@PrepareForTest({ FileSystemHousekeepingService.class, FileSystem.class })
 public class FileSystemHousekeepingServiceTest {
 
   @Rule
@@ -93,7 +98,7 @@ public void init() throws Exception {
     val1Path = new Path(tmpFolder.newFolder("foo", "bar", PATH_EVENT_ID, "test=1", "val=1").getCanonicalPath());
     val2Path = new Path(tmpFolder.newFolder("foo", "bar", PATH_EVENT_ID, "test=1", "val=2").getCanonicalPath());
     val3Path = new Path(tmpFolder.newFolder("foo", "bar", PATH_EVENT_ID, "test=1", "val=3").getCanonicalPath());
-    service = new FileSystemHousekeepingService(legacyReplicationPathRepository, conf);
+    service = PowerMockito.spy(new FileSystemHousekeepingService(legacyReplicationPathRepository, conf));
     cleanUpPath1 = new HousekeepingLegacyReplicaPath(EVENT_ID, PATH_EVENT_ID, val1Path.toString());
     cleanUpPath2 = new HousekeepingLegacyReplicaPath(EVENT_ID, PATH_EVENT_ID, val2Path.toString());
     cleanUpPath3 = new HousekeepingLegacyReplicaPath(EVENT_ID, PATH_EVENT_ID, val3Path.toString());
@@ -124,6 +129,79 @@ public void cleanUp() throws Exception {
     deleted(eventPath);
   }
 
+  @Test
+  public void housekeepingPathsWithOneFileSystemLoadFailureCleansUpOtherPaths() throws Exception {
+    PowerMockito.doReturn(null).when(service, "fileSystemForPath", eq(new Path(cleanUpPath1.getPath())));
+    PowerMockito.doReturn(spyFs).when(service, "fileSystemForPath", eq(new Path(cleanUpPath2.getPath())));
+    PowerMockito.doReturn(spyFs).when(service, "fileSystemForPath", eq(new Path(cleanUpPath3.getPath())));
+
+    when(legacyReplicationPathRepository.findByCreationTimestampLessThanEqual(now.getMillis()))
+        .thenReturn(Arrays.asList(cleanUpPath1, cleanUpPath2, cleanUpPath3));
+
+    service.cleanUp(now);
+
+    verify(spyFs, times(0)).delete(eq(new Path(cleanUpPath1.getPath())), eq(true));
+    verify(spyFs).delete(eq(new Path(cleanUpPath2.getPath())), eq(true));
+    verify(spyFs).delete(eq(new Path(cleanUpPath3.getPath())), eq(true));
+
+    verify(legacyReplicationPathRepository, times(0)).delete(cleanUpPath1);
+    verify(legacyReplicationPathRepository).delete(cleanUpPath2);
+    verify(legacyReplicationPathRepository).delete(cleanUpPath3);
+  }
+
+  @Test
+  public void housekeepingPathThatDoesntExistSkipsDeleteAndRemovesPathFromHousekeepingDatabase() throws Exception {
+    when(legacyReplicationPathRepository.findByCreationTimestampLessThanEqual(now.getMillis()))
+        .thenReturn(Arrays.asList(cleanUpPath1));
+    doReturn(false).when(spyFs).exists(any(Path.class));
+
+    service.cleanUp(now);
+
+    verify(spyFs, times(0)).delete(eq(new Path(cleanUpPath1.getPath())), eq(true));
+    verifyPrivate(service).invoke("deleteParents", eq(spyFs), eq(new Path(cleanUpPath1.getPath())), anyString());
+    verify(legacyReplicationPathRepository, times(1)).delete(cleanUpPath1);
+  }
+
+  @Test
+  public void deleteFailureDoesntRemovePathFromHousekeepingDatabase() throws Exception {
+    when(legacyReplicationPathRepository.findByCreationTimestampLessThanEqual(now.getMillis()))
+        .thenReturn(Arrays.asList(cleanUpPath1));
+    doThrow(new RuntimeException()).when(spyFs).delete(eq(new Path(cleanUpPath1.getPath())), eq(true));
+
+    service.cleanUp(now);
+
+    verify(spyFs, times(1)).delete(eq(new Path(cleanUpPath1.getPath())), eq(true));
+    verify(legacyReplicationPathRepository, times(0)).delete(cleanUpPath1);
+  }
+
+  @Test
+  public void siblingCheckFailureDoesntFailHousekeeping() throws Exception {
+    PowerMockito.doThrow(new RuntimeException()).when(service, "oneOfMySiblingsWillTakeCareOfMyAncestors",
+        any(Path.class), any(Path.class), any(FileSystem.class));
+
+    when(legacyReplicationPathRepository.findByCreationTimestampLessThanEqual(now.getMillis()))
+        .thenReturn(Arrays.asList(cleanUpPath1));
+
+    service.cleanUp(now);
+
+    verify(legacyReplicationPathRepository, times(0)).delete(cleanUpPath1);
+  }
+
+  @Test
+  public void nothingMoreToDeleteFailureDoesntFailHousekeeping() throws Exception {
+    PowerMockito.doThrow(new RuntimeException()).when(service, "thereIsNothingMoreToDelete", any(FileSystem.class),
+        any(Path.class));
+    PowerMockito.doReturn(false).when(service, "oneOfMySiblingsWillTakeCareOfMyAncestors", any(Path.class),
+        any(Path.class), any(FileSystem.class));
+
+    when(legacyReplicationPathRepository.findByCreationTimestampLessThanEqual(now.getMillis()))
+        .thenReturn(Arrays.asList(cleanUpPath1));
+
+    service.cleanUp(now);
+
+    verify(legacyReplicationPathRepository, times(0)).delete(cleanUpPath1);
+  }
+
   @Test
   public void eventuallyConsistentCleanUpFull() throws Exception {
     when(legacyReplicationPathRepository.findByCreationTimestampLessThanEqual(now.getMillis()))
@@ -201,7 +279,7 @@ public void eventuallyConsistentCleanUpOnlyKeys() throws Exception {
         .thenReturn(Arrays.asList(cleanUpPath4));
 
     doCallRealMethod().when(spyFs).delete(val4Path, true);
-    doReturn(false).when(spyFs).exists(val4Path);
+    doCallRealMethod().when(spyFs).exists(val4Path);
     doReturn(false).when(spyFs).exists(eventPath);
     service.cleanUp(now);
 
@@ -253,8 +331,8 @@ public void filesystemParentPathDeletionFailsKeepsCleanUpPathForRetry() throws E
     deleted(val1Path, val2Path, val3Path);
   }
 
-  @Test(expected = RuntimeException.class)
-  public void repositoryDeletionFails() throws Exception {
+  @Test
+  public void repositoryDeletionFailsHousekeepingContinues() throws Exception {
     doThrow(new RuntimeException()).when(legacyReplicationPathRepository).delete(cleanUpPath1);
     when(legacyReplicationPathRepository.findByCreationTimestampLessThanEqual(now.getMillis()))
         .thenReturn(Arrays.asList(cleanUpPath1));