diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java index 1d46e57..a440f35 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java @@ -121,7 +121,7 @@ public abstract class CleanerChore extends Chore // loop over the found files and see if they should be deleted for (FileStatus file : files) { try { - if (file.isDir()) checkDirectory(file.getPath()); + if (file.isDir()) checkAndDeleteDirectory(file.getPath()); else checkAndDelete(file.getPath()); } catch (IOException e) { e = RemoteExceptionHandler.checkIOException(e); @@ -135,59 +135,40 @@ public abstract class CleanerChore extends Chore } /** - * Check to see if we can delete a directory (and all the children files of that directory). + * Attempt to delete a directory and all files under that directory. Each child file is passed + * through the delegates to see if it can be deleted. If the directory has not children when the + * cleaners have finished it is deleted. *

- * A directory will not be deleted if it has children that are subsequently deleted since that - * will require another set of lookups in the filesystem, which is semantically same as waiting - * until the next time the chore is run, so we might as well wait. - * @param fs {@link FileSystem} where he directory resides + * If new children files are added between checks of the directory, the directory will not + * be deleted. * @param toCheck directory to check - * @throws IOException + * @return true if the directory was deleted, false otherwise. + * @throws IOException if there is an unexpected filesystem error */ - private void checkDirectory(Path toCheck) throws IOException { + private boolean checkAndDeleteDirectory(Path toCheck) throws IOException { LOG.debug("Checking directory: " + toCheck); - FileStatus[] files = checkAndDeleteDirectory(toCheck); + FileStatus[] children = FSUtils.listStatus(fs, toCheck); // if the directory doesn't exist, then we are done - if (files == null) return; - - // otherwise we need to check each of the child files - for (FileStatus file : files) { - Path filePath = file.getPath(); - // if its a directory, then check to see if it should be deleted - if (file.isDir()) { - // check the subfiles to see if they can be deleted - checkDirectory(filePath); - continue; + if (children == null) return true; + + boolean canDeleteThis = true; + for (FileStatus child : children) { + Path path = child.getPath(); + // attempt to delete all the files under the directory + if (child.isDir()) { + if (!checkAndDeleteDirectory(path)) { + canDeleteThis = false; + } } // otherwise we can just check the file - checkAndDelete(filePath); - } - - // recheck the directory to see if we can delete it this time - checkAndDeleteDirectory(toCheck); - } - - /** - * Check and delete the passed directory if the directory is empty - * @param toCheck full path to the directory to check (and possibly delete) - * @return null if the directory was empty (and possibly deleted) and otherwise an array - * of FileStatus for the files in the directory - * @throws IOException - */ - private FileStatus[] checkAndDeleteDirectory(Path toCheck) throws IOException { - LOG.debug("Attempting to delete directory:" + toCheck); - // if it doesn't exist, we are done - if (!fs.exists(toCheck)) return null; - // get the files below the directory - FileStatus[] files = FSUtils.listStatus(fs, toCheck, null); - // if there are no subfiles, then we can delete the directory - if (files == null) { - checkAndDelete(toCheck); - return null; + else if (!checkAndDelete(path)) { + canDeleteThis = false; + } } - // return the status of the files in the directory - return files; + // if all the children have been deleted, then we should try to delete this directory. However, + // don't do so recursively so we don't delete files that have been added since we checked. + return canDeleteThis ? fs.delete(toCheck, false) : false; } /** @@ -197,34 +178,40 @@ public abstract class CleanerChore extends Chore * @throws IOException if cann't delete a file because of a filesystem issue * @throws IllegalArgumentException if the file is a directory and has children */ - private void checkAndDelete(Path filePath) throws IOException, IllegalArgumentException { + private boolean checkAndDelete(Path filePath) throws IOException, IllegalArgumentException { + // first check to see if the path is valid if (!validate(filePath)) { LOG.warn("Found a wrongly formatted file: " + filePath.getName() + " deleting it."); - if (!this.fs.delete(filePath, true)) { + boolean success = this.fs.delete(filePath, true); + if(!success) LOG.warn("Attempted to delete:" + filePath + ", but couldn't. Run cleaner chain and attempt to delete on next pass."); - } - return; + + return success; } + + // check each of the cleaners for the file for (T cleaner : cleanersChain) { - if (cleaner.isStopped()) { + if (cleaner.isStopped() || this.stopper.isStopped()) { LOG.warn("A file cleaner" + this.getName() + " is stopped, won't delete any file in:" + this.oldFileDir); - return; + return false; } if (!cleaner.isFileDeletable(filePath)) { // this file is not deletable, then we are done LOG.debug(filePath + " is not deletable according to:" + cleaner); - return; + return false; } } // delete this file if it passes all the cleaners LOG.debug("Removing:" + filePath + " from archive"); - if (!this.fs.delete(filePath, false)) { + boolean success = this.fs.delete(filePath, false); + if (!success) { LOG.warn("Attempted to delete:" + filePath + ", but couldn't. Run cleaner chain and attempt to delete on next pass."); } + return success; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java new file mode 100644 index 0000000..8503caf --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java @@ -0,0 +1,255 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.hadoop.hbase.master.cleaner; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.SmallTests; +import org.apache.hadoop.hbase.Stoppable; +import org.apache.hadoop.hbase.util.FSUtils; +import org.apache.hadoop.hbase.util.StoppableImplementation; +import org.junit.After; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.mockito.Mockito; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +@Category(SmallTests.class) +public class TestCleanerChore { + + private static final Log LOG = LogFactory.getLog(TestCleanerChore.class); + private static final HBaseTestingUtility UTIL = new HBaseTestingUtility(); + + @After + public void cleanup() throws Exception { + // delete and recreate the test directory, ensuring a clean test dir between tests + Path testDir = UTIL.getDataTestDir(); + FileSystem fs = UTIL.getTestFileSystem(); + fs.delete(testDir, true); + fs.mkdirs(testDir); + } + + @Test + public void testSavesFilesOnRequest() throws Exception { + Stoppable stop = new StoppableImplementation(); + Configuration conf = UTIL.getConfiguration(); + Path testDir = UTIL.getDataTestDir(); + FileSystem fs = UTIL.getTestFileSystem(); + String confKey = "hbase.test.cleaner.delegates"; + conf.set(confKey, NeverDelete.class.getName()); + + AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + + // create the directory layout in the directory to clean + Path parent = new Path(testDir, "parent"); + Path file = new Path(parent, "someFile"); + fs.mkdirs(parent); + // touch a new file + fs.create(file).close(); + assertTrue("Test file didn't get created.", fs.exists(file)); + + // run the chore + chore.chore(); + + // verify all the files got deleted + assertTrue("File didn't get deleted", fs.exists(file)); + assertTrue("Empty directory didn't get deleted", fs.exists(parent)); + } + + @Test + public void testDeletesEmptyDirectories() throws Exception { + Stoppable stop = new StoppableImplementation(); + Configuration conf = UTIL.getConfiguration(); + Path testDir = UTIL.getDataTestDir(); + FileSystem fs = UTIL.getTestFileSystem(); + String confKey = "hbase.test.cleaner.delegates"; + conf.set(confKey, AlwaysDelete.class.getName()); + + AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + + // create the directory layout in the directory to clean + Path parent = new Path(testDir, "parent"); + Path child = new Path(parent, "child"); + Path file = new Path(child, "someFile"); + fs.mkdirs(child); + // touch a new file + fs.create(file).close(); + // also create a file in the top level directory + Path topFile = new Path(testDir, "topFile"); + fs.create(topFile).close(); + assertTrue("Test file didn't get created.", fs.exists(file)); + assertTrue("Test file didn't get created.", fs.exists(topFile)); + + // run the chore + chore.chore(); + + // verify all the files got deleted + assertFalse("File didn't get deleted", fs.exists(topFile)); + assertFalse("File didn't get deleted", fs.exists(file)); + assertFalse("Empty directory didn't get deleted", fs.exists(child)); + assertFalse("Empty directory didn't get deleted", fs.exists(parent)); + } + + /** + * Test to make sure that we don't attempt to ask the delegate whether or not we should preserve a + * directory. + * @throws Exception on failure + */ + @Test + public void testDoesNotCheckDirectories() throws Exception { + Stoppable stop = new StoppableImplementation(); + Configuration conf = UTIL.getConfiguration(); + Path testDir = UTIL.getDataTestDir(); + FileSystem fs = UTIL.getTestFileSystem(); + String confKey = "hbase.test.cleaner.delegates"; + conf.set(confKey, AlwaysDelete.class.getName()); + + AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + // spy on the delegate to ensure that we don't check for directories + AlwaysDelete delegate = (AlwaysDelete) chore.cleanersChain.get(0); + AlwaysDelete spy = Mockito.spy(delegate); + chore.cleanersChain.set(0, spy); + + // create the directory layout in the directory to clean + Path parent = new Path(testDir, "parent"); + Path file = new Path(parent, "someFile"); + fs.mkdirs(parent); + // touch a new file + fs.create(file).close(); + assertTrue("Test file didn't get created.", fs.exists(file)); + + chore.chore(); + // make sure we never checked the directory + Mockito.verify(spy, Mockito.never()).isFileDeletable(parent); + Mockito.reset(spy); + } + + @Test + public void testStoppedCleanerDoesNotDeleteFiles() throws Exception { + Stoppable stop = new StoppableImplementation(); + Configuration conf = UTIL.getConfiguration(); + Path testDir = UTIL.getDataTestDir(); + FileSystem fs = UTIL.getTestFileSystem(); + String confKey = "hbase.test.cleaner.delegates"; + conf.set(confKey, AlwaysDelete.class.getName()); + + AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + + // also create a file in the top level directory + Path topFile = new Path(testDir, "topFile"); + fs.create(topFile).close(); + assertTrue("Test file didn't get created.", fs.exists(topFile)); + + // stop the chore + stop.stop("testing stop"); + + // run the chore + chore.chore(); + + // test that the file still exists + assertTrue("File got deleted while chore was stopped", fs.exists(topFile)); + } + + /** + * While cleaning a directory, all the files in the directory may be deleted, but there may be + * another file added, in which case the directory shouldn't be deleted. + * @throws IOException on failure + */ + @Test + public void testCleanerDoesNotDeleteDirectoryWithLateAddedFiles() throws IOException { + Stoppable stop = new StoppableImplementation(); + Configuration conf = UTIL.getConfiguration(); + final Path testDir = UTIL.getDataTestDir(); + final FileSystem fs = UTIL.getTestFileSystem(); + String confKey = "hbase.test.cleaner.delegates"; + conf.set(confKey, AlwaysDelete.class.getName()); + + AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + // spy on the delegate to ensure that we don't check for directories + AlwaysDelete delegate = (AlwaysDelete) chore.cleanersChain.get(0); + AlwaysDelete spy = Mockito.spy(delegate); + chore.cleanersChain.set(0, spy); + + // create the directory layout in the directory to clean + final Path parent = new Path(testDir, "parent"); + Path file = new Path(parent, "someFile"); + fs.mkdirs(parent); + // touch a new file + fs.create(file).close(); + assertTrue("Test file didn't get created.", fs.exists(file)); + final Path addedFile = new Path(parent, "addedFile"); + + // when we attempt to delete the original file, add another file in the same directory + Mockito.doAnswer(new Answer() { + @Override + public Boolean answer(InvocationOnMock invocation) throws Throwable { + fs.create(addedFile).close(); + FSUtils.logFileSystemState(fs, testDir, LOG); + return (Boolean) invocation.callRealMethod(); + } + }).when(spy).isFileDeletable(Mockito.any(Path.class)); + + // run the chore + chore.chore(); + + // make sure all the directories + added file exist, but the original file is deleted + assertTrue("Added file unexpectedly deleted", fs.exists(addedFile)); + assertTrue("Parent directory deleted unexpectedly", fs.exists(parent)); + assertFalse("Original file unexpectedly retained", fs.exists(file)); + Mockito.verify(spy, Mockito.times(1)).isFileDeletable(Mockito.any(Path.class)); + Mockito.reset(spy); + } + + private static class AllValidPaths extends CleanerChore { + + public AllValidPaths(String name, Stoppable s, Configuration conf, FileSystem fs, + Path oldFileDir, String confkey) { + super(name, Integer.MAX_VALUE, s, conf, fs, oldFileDir, confkey); + } + + // all paths are valid + @Override + protected boolean validate(Path file) { + return true; + } + }; + + public static class AlwaysDelete extends BaseHFileCleanerDelegate { + @Override + public boolean isFileDeletable(Path file) { + return true; + } + } + + public static class NeverDelete extends BaseHFileCleanerDelegate { + @Override + public boolean isFileDeletable(Path file) { + return false; + } + } +} \ No newline at end of file