From b4466a3b0a772d53e948f3e440f3e8e285f12a26 Mon Sep 17 00:00:00 2001 From: Gopal V Date: Wed, 17 Jul 2019 13:49:20 +0100 Subject: [PATCH] HADOOP-16341. ShutDownHookManager: Regressed performance on Hook removals after HADOOP-15679 Contributed by Gopal V and Atilla Magyar. Change-Id: I066d5eece332a1673594de0f9b484443f95530ec --- .../hadoop/util/ShutdownHookManager.java | 21 +++++++++----- .../hadoop/util/TestShutdownHookManager.java | 29 ++++++++++++------- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ShutdownHookManager.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ShutdownHookManager.java index 556f4e0656e..76d90063609 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ShutdownHookManager.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ShutdownHookManager.java @@ -92,7 +92,7 @@ public final class ShutdownHookManager { return; } long started = System.currentTimeMillis(); - int timeoutCount = executeShutdown(); + int timeoutCount = MGR.executeShutdown(); long ended = System.currentTimeMillis(); LOG.debug(String.format( "Completed shutdown in %.3f seconds; Timeouts: %d", @@ -116,9 +116,9 @@ public final class ShutdownHookManager { */ @InterfaceAudience.Private @VisibleForTesting - static int executeShutdown() { + int executeShutdown() { int timeouts = 0; - for (HookEntry entry: MGR.getShutdownHooksInOrder()) { + for (HookEntry entry: getShutdownHooksInOrder()) { Future future = EXECUTOR.submit(entry.getHook()); try { future.get(entry.getTimeout(), entry.getTimeUnit()); @@ -254,7 +254,9 @@ public final class ShutdownHookManager { private AtomicBoolean shutdownInProgress = new AtomicBoolean(false); //private to constructor to ensure singularity - private ShutdownHookManager() { + @VisibleForTesting + @InterfaceAudience.Private + ShutdownHookManager() { } /** @@ -267,8 +269,8 @@ public final class ShutdownHookManager { @VisibleForTesting List getShutdownHooksInOrder() { List list; - synchronized (MGR.hooks) { - list = new ArrayList<>(MGR.hooks); + synchronized (hooks) { + list = new ArrayList<>(hooks); } Collections.sort(list, new Comparator() { @@ -342,7 +344,9 @@ public final class ShutdownHookManager { throw new IllegalStateException("Shutdown in progress, cannot remove a " + "shutdownHook"); } - return hooks.remove(new HookEntry(shutdownHook, 0)); + // hooks are only == by runnable + return hooks.remove(new HookEntry(shutdownHook, 0, TIMEOUT_MINIMUM, + TIME_UNIT_DEFAULT)); } /** @@ -354,7 +358,8 @@ public final class ShutdownHookManager { @InterfaceAudience.Public @InterfaceStability.Stable public boolean hasShutdownHook(Runnable shutdownHook) { - return hooks.contains(new HookEntry(shutdownHook, 0)); + return hooks.contains(new HookEntry(shutdownHook, 0, TIMEOUT_MINIMUM, + TIME_UNIT_DEFAULT)); } /** diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShutdownHookManager.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShutdownHookManager.java index 03fa903170f..59430e8bd94 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShutdownHookManager.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShutdownHookManager.java @@ -21,7 +21,6 @@ import java.util.List; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import org.junit.After; import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -43,13 +42,10 @@ public class TestShutdownHookManager { LoggerFactory.getLogger(TestShutdownHookManager.class.getName()); /** - * remove all the shutdown hooks so that they never get invoked later - * on in this test process. + * A new instance of ShutdownHookManager to ensure parallel tests + * don't have shared context. */ - @After - public void clearShutdownHooks() { - ShutdownHookManager.get().clearShutdownHooks(); - } + private final ShutdownHookManager mgr = new ShutdownHookManager(); /** * Verify hook registration, then execute the hook callback stage @@ -58,7 +54,6 @@ public class TestShutdownHookManager { */ @Test public void shutdownHookManager() { - ShutdownHookManager mgr = ShutdownHookManager.get(); assertNotNull("No ShutdownHookManager", mgr); assertEquals(0, mgr.getShutdownHooksInOrder().size()); Hook hook1 = new Hook("hook1", 0, false); @@ -119,7 +114,7 @@ public class TestShutdownHookManager { // now execute the hook shutdown sequence INVOCATION_COUNT.set(0); LOG.info("invoking executeShutdown()"); - int timeouts = ShutdownHookManager.executeShutdown(); + int timeouts = mgr.executeShutdown(); LOG.info("Shutdown completed"); assertEquals("Number of timed out hooks", 1, timeouts); @@ -193,7 +188,6 @@ public class TestShutdownHookManager { */ @Test public void testDuplicateRegistration() throws Throwable { - ShutdownHookManager mgr = ShutdownHookManager.get(); Hook hook = new Hook("hook1", 0, false); // add the hook @@ -222,6 +216,21 @@ public class TestShutdownHookManager { } + @Test + public void testShutdownRemove() throws Throwable { + assertNotNull("No ShutdownHookManager", mgr); + assertEquals(0, mgr.getShutdownHooksInOrder().size()); + Hook hook1 = new Hook("hook1", 0, false); + Hook hook2 = new Hook("hook2", 0, false); + mgr.addShutdownHook(hook1, 9); // create Hook1 with priority 9 + assertTrue("No hook1", mgr.hasShutdownHook(hook1)); // hook1 lookup works + assertEquals(1, mgr.getShutdownHooksInOrder().size()); // 1 hook + assertFalse("Delete hook2 should not be allowed", + mgr.removeShutdownHook(hook2)); + assertTrue("Can't delete hook1", mgr.removeShutdownHook(hook1)); + assertEquals(0, mgr.getShutdownHooksInOrder().size()); + } + private static final AtomicInteger INVOCATION_COUNT = new AtomicInteger(); /**