From 37ae042483c80da0182b751fabee411ee15c65d3 Mon Sep 17 00:00:00 2001 From: Reid Chan Date: Wed, 20 Jun 2018 12:33:38 +0800 Subject: [PATCH] HBASE-20732 Shutdown scan pool when master is stopped --- .../org/apache/hadoop/hbase/master/HMaster.java | 1 + .../hadoop/hbase/master/cleaner/CleanerChore.java | 13 +- .../hbase/master/cleaner/TestCleanerChore.java | 244 ++++++++++++++------- 3 files changed, 173 insertions(+), 85 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 883bb4ff52..2d8e530f0a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -1240,6 +1240,7 @@ public class HMaster extends HRegionServer implements MasterServices { } super.stopServiceThreads(); stopChores(); + CleanerChore.shutDownChorePool(); LOG.debug("Stopping service threads"); 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 bcb23ac29e..c4e1e379e3 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 @@ -118,7 +118,7 @@ public abstract class CleanerChore extends Schedu break; } } - pool.shutdownNow(); + shutDownNow(); LOG.info("Update chore's pool size from {} to {}", pool.getParallelism(), size); pool = new ForkJoinPool(size); } @@ -136,6 +136,13 @@ public abstract class CleanerChore extends Schedu synchronized void submit(ForkJoinTask task) { pool.submit(task); } + + synchronized void shutDownNow() { + if (pool == null || pool.isShutdown()) { + return; + } + pool.shutdownNow(); + } } // It may be waste resources for each cleaner chore own its pool, // so let's make pool for all cleaner chores. @@ -154,6 +161,10 @@ public abstract class CleanerChore extends Schedu } } + public static void shutDownChorePool() { + POOL.shutDownNow(); + } + public CleanerChore(String name, final int sleepPeriod, final Stoppable s, Configuration conf, FileSystem fs, Path oldFileDir, String confKey) { this(name, sleepPeriod, s, conf, fs, oldFileDir, confKey, null); 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 index 6ce941f029..be3a04221b 100644 --- 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 @@ -30,6 +30,7 @@ import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.FilterFileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.ChoreService; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.Stoppable; @@ -37,8 +38,9 @@ import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.apache.hadoop.hbase.util.FSUtils; import org.apache.hadoop.hbase.util.StoppableImplementation; -import org.junit.After; -import org.junit.Before; +import org.apache.hadoop.hbase.util.Threads; +import org.junit.AfterClass; +import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -57,16 +59,19 @@ public class TestCleanerChore { private static final Logger LOG = LoggerFactory.getLogger(TestCleanerChore.class); private static final HBaseTestingUtility UTIL = new HBaseTestingUtility(); + private static int PERIOD = 500; + private static int SLEEP_PERIOD = 600; - @Before - public void setup() throws Exception { + @BeforeClass + public static void setup() { CleanerChore.initChorePool(UTIL.getConfiguration()); } - @After - public void cleanup() throws Exception { + @AfterClass + public static void cleanup() throws Exception { // delete and recreate the test directory, ensuring a clean test dir between tests UTIL.cleanupTestDir(); + CleanerChore.shutDownChorePool(); } @Test @@ -77,8 +82,8 @@ public class TestCleanerChore { 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); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, stop, + conf, fs, testDir, confKey); // create the directory layout in the directory to clean Path parent = new Path(testDir, "parent"); @@ -88,12 +93,17 @@ public class TestCleanerChore { fs.create(file).close(); assertTrue("Test file didn't get created.", fs.exists(file)); - // run the chore - chore.chore(); - - // verify all the files were preserved - assertTrue("File shouldn't have been deleted", fs.exists(file)); - assertTrue("directory shouldn't have been deleted", fs.exists(parent)); + ChoreService service = new ChoreService("testSavesFilesOnRequest"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + // verify all the files were preserved + assertTrue("File shouldn't have been deleted", fs.exists(file)); + assertTrue("directory shouldn't have been deleted", fs.exists(parent)); + } finally { + shutdownService(service); + } } @Test @@ -151,7 +161,8 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, stop, + conf, fs, testDir, confKey); // create the directory layout in the directory to clean Path parent = new Path(testDir, "parent"); @@ -168,14 +179,20 @@ public class TestCleanerChore { 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)); + ChoreService service = new ChoreService("testDeletesEmptyDirectories"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + + // 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)); + } finally { + shutdownService(service); + } } /** @@ -192,7 +209,8 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, 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); @@ -208,10 +226,17 @@ public class TestCleanerChore { assertTrue("Test file didn't get created.", fs.exists(file)); FileStatus fStat = fs.getFileStatus(parent); - chore.chore(); - // make sure we never checked the directory - Mockito.verify(spy, Mockito.never()).isFileDeletable(fStat); - Mockito.reset(spy); + ChoreService service = new ChoreService("testDoesNotCheckDirectories"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + // make sure we never checked the directory + Mockito.verify(spy, Mockito.never()).isFileDeletable(fStat); + Mockito.reset(spy); + } finally { + shutdownService(service); + } } @Test @@ -223,21 +248,27 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, 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)); + ChoreService service = new ChoreService("testStoppedCleanerDoesNotDeleteFiles"); + try { + // run the chore. + service.scheduleChore(chore); + // stop the chore + stop.stop("testing stop"); + Threads.sleep(SLEEP_PERIOD); + + // test that the file still exists + assertTrue("File got deleted while chore was stopped", fs.exists(topFile)); + } finally { + shutdownService(service); + } } /** @@ -246,7 +277,7 @@ public class TestCleanerChore { * @throws IOException on failure */ @Test - public void testCleanerDoesNotDeleteDirectoryWithLateAddedFiles() throws IOException { + public void testCleanerDoesNotDeleteDirectoryWithLateAddedFiles() throws Exception { Stoppable stop = new StoppableImplementation(); Configuration conf = UTIL.getConfiguration(); final Path testDir = UTIL.getDataTestDir(); @@ -254,7 +285,8 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, 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); @@ -279,15 +311,22 @@ public class TestCleanerChore { } }).when(spy).isFileDeletable(Mockito.any()); - // 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()); - Mockito.reset(spy); + ChoreService service = + new ChoreService("testCleanerDoesNotDeleteDirectoryWithLateAddedFiles"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + + // 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()); + Mockito.reset(spy); + } finally { + shutdownService(service); + } } /** @@ -312,7 +351,8 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, 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); @@ -337,14 +377,20 @@ public class TestCleanerChore { } }).when(spy).isFileDeletable(Mockito.any()); - // 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(racyFile)); - 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()); + ChoreService service = + new ChoreService("testNoExceptionFromDirectoryWithRacyChildren"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + + // make sure all the directories + added file exist, but the original file is deleted + assertTrue("Added file unexpectedly deleted", fs.exists(racyFile)); + assertTrue("Parent directory deleted unexpectedly", fs.exists(parent)); + assertFalse("Original file unexpectedly retained", fs.exists(file)); + } finally { + shutdownService(service); + } } @Test @@ -356,7 +402,8 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, stop, + conf, fs, testDir, confKey); // Enable cleaner chore.setEnabled(true); @@ -371,13 +418,19 @@ public class TestCleanerChore { 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 - 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)); + ChoreService service = new ChoreService("testDeleteFileWithCleanerEnabled"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + + // verify all the files got deleted + 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)); + } finally { + shutdownService(service); + } } @Test @@ -389,7 +442,8 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, stop, + conf, fs, testDir, confKey); // Disable cleaner chore.setEnabled(false); @@ -404,13 +458,19 @@ public class TestCleanerChore { fs.create(file).close(); assertTrue("Test file didn't get created.", fs.exists(file)); - // run the chore - chore.chore(); - - // verify all the files exist - assertTrue("File got deleted with cleaner disabled", fs.exists(file)); - assertTrue("Directory got deleted", fs.exists(child)); - assertTrue("Directory got deleted", fs.exists(parent)); + ChoreService service = new ChoreService("testDeleteFileWithCleanerEnabled"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + + // verify all the files exist + assertTrue("File got deleted with cleaner disabled", fs.exists(file)); + assertTrue("Directory got deleted", fs.exists(child)); + assertTrue("Directory got deleted", fs.exists(parent)); + } finally { + shutdownService(service); + } } @Test @@ -431,7 +491,8 @@ public class TestCleanerChore { String confKey = "hbase.test.cleaner.delegates"; conf.set(confKey, AlwaysDelete.class.getName()); conf.set(CleanerChore.CHORE_POOL_SIZE, String.valueOf(initPoolSize)); - AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey); + AllValidPaths chore = new AllValidPaths("test-file-cleaner", PERIOD, stop, + conf, fs, testDir, confKey); chore.setEnabled(true); // Create subdirs under testDir int dirNums = 6; @@ -444,16 +505,19 @@ public class TestCleanerChore { for (Path subdir : subdirs) { createFiles(fs, subdir, 6); } - // Start chore - Thread t = new Thread(() -> chore.chore()); - t.setDaemon(true); - t.start(); - // Change size of chore's pool - conf.set(CleanerChore.CHORE_POOL_SIZE, String.valueOf(changedPoolSize)); - chore.onConfigurationChange(conf); - assertEquals(changedPoolSize, chore.getChorePoolSize()); - // Stop chore - t.join(); + ChoreService service = new ChoreService("testOnConfigurationChange"); + try { + // run the chore. + service.scheduleChore(chore); + Threads.sleep(SLEEP_PERIOD); + + // Change size of chore's pool + conf.set(CleanerChore.CHORE_POOL_SIZE, String.valueOf(changedPoolSize)); + chore.onConfigurationChange(conf); + assertEquals(changedPoolSize, chore.getChorePoolSize()); + } finally { + shutdownService(service); + } } @Test @@ -489,6 +553,13 @@ public class TestCleanerChore { } } + private void shutdownService(ChoreService service) throws InterruptedException { + service.shutdown(); + while (!service.isTerminated()) { + Thread.sleep(100); + } + } + private static class AllValidPaths extends CleanerChore { public AllValidPaths(String name, Stoppable s, Configuration conf, FileSystem fs, @@ -496,6 +567,11 @@ public class TestCleanerChore { super(name, Integer.MAX_VALUE, s, conf, fs, oldFileDir, confkey); } + public AllValidPaths(String name, int sleepPeriod, Stoppable s, + Configuration conf, FileSystem fs, Path oldFileDir, String confkey) { + super(name, sleepPeriod, s, conf, fs, oldFileDir, confkey); + } + // all paths are valid @Override protected boolean validate(Path file) { -- 2.15.0