diff --git a/CHANGELOG.md b/CHANGELOG.md index b485e53..4b7b9ac 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/main/java/com/hotels/housekeeping/HousekeepingException.java b/src/main/java/com/hotels/housekeeping/HousekeepingException.java new file mode 100644 index 0000000..1789796 --- /dev/null +++ b/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); + } + +} diff --git a/src/main/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingService.java b/src/main/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingService.java index 7b95399..76fb250 100644 --- a/src/main/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingService.java +++ b/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,6 +48,50 @@ 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 { @@ -54,32 +99,11 @@ public void cleanUp(Instant referenceTime) { .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); } } diff --git a/src/test/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingServiceTest.java b/src/test/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingServiceTest.java index 347e943..6f3f608 100644 --- a/src/test/java/com/hotels/housekeeping/service/impl/FileSystemHousekeepingServiceTest.java +++ b/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));