From 6da6f0ac0d7ce3a7f816fbe6f64f82e714103a3d Mon Sep 17 00:00:00 2001 From: Attila Magyar Date: Fri, 12 Jul 2019 16:48:58 +0200 Subject: [PATCH 1/4] HADOOP-16341. ShutDownHookManager: Regressed performance on Hook removals after HADOOP-15679 --- .../hadoop/util/ShutdownHookManager.java | 15 +++++++---- .../hadoop/util/TestShutdownHookManager.java | 25 +++++++++++++------ 2 files changed, 27 insertions(+), 13 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 2ca8e55f0bd92..bea6cace46dc9 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 @@ -254,7 +254,9 @@ TimeUnit getTimeUnit() { private AtomicBoolean shutdownInProgress = new AtomicBoolean(false); //private to constructor to ensure singularity - private ShutdownHookManager() { + @VisibleForTesting + @InterfaceAudience.Private + ShutdownHookManager() { } /** @@ -267,8 +269,8 @@ private 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 boolean removeShutdownHook(Runnable shutdownHook) { 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 boolean removeShutdownHook(Runnable shutdownHook) { @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 03fa903170f2f..13039fbc2be7b 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 @@ -43,13 +43,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 +55,6 @@ public void clearShutdownHooks() { */ @Test public void shutdownHookManager() { - ShutdownHookManager mgr = ShutdownHookManager.get(); assertNotNull("No ShutdownHookManager", mgr); assertEquals(0, mgr.getShutdownHooksInOrder().size()); Hook hook1 = new Hook("hook1", 0, false); @@ -193,7 +189,6 @@ public void testShutdownTimeoutBadConfiguration() throws Throwable { */ @Test public void testDuplicateRegistration() throws Throwable { - ShutdownHookManager mgr = ShutdownHookManager.get(); Hook hook = new Hook("hook1", 0, false); // add the hook @@ -222,6 +217,20 @@ public void testDuplicateRegistration() throws Throwable { } + @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(mgr.hasShutdownHook(hook1)); // hook1 lookup works + assertEquals(1, mgr.getShutdownHooksInOrder().size()); // 1 hook + assertFalse(mgr.removeShutdownHook(hook2)); // can't delete hook2 + assertTrue(mgr.removeShutdownHook(hook1)); // can delete hook1 + assertEquals(0, mgr.getShutdownHooksInOrder().size()); // no more hooks + } + private static final AtomicInteger INVOCATION_COUNT = new AtomicInteger(); /** From d29a4468605336b2d9904602fc419fa6f7be9ecc Mon Sep 17 00:00:00 2001 From: Attila Magyar Date: Sat, 13 Jul 2019 16:06:06 +0200 Subject: [PATCH 2/4] HADOOP-16341. ShutDownHookManager: Regressed performance on Hook removals after HADOOP-15679 --- .../java/org/apache/hadoop/util/ShutdownHookManager.java | 6 +++--- .../org/apache/hadoop/util/TestShutdownHookManager.java | 2 +- 2 files changed, 4 insertions(+), 4 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 bea6cace46dc9..3b8d577cb00f4 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 void run() { 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 void run() { */ @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()); 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 13039fbc2be7b..c8e6d4941b0b2 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 @@ -115,7 +115,7 @@ public void shutdownHookManager() { // 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); From 22e22695adfa1e30169758ad572744f599284092 Mon Sep 17 00:00:00 2001 From: Attila Magyar Date: Mon, 15 Jul 2019 14:17:42 +0200 Subject: [PATCH 3/4] HADOOP-16341. ShutDownHookManager: Regressed performance on Hook removals after HADOOP-15679 --- .../org/apache/hadoop/util/TestShutdownHookManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 c8e6d4941b0b2..ae916aa0c8b0e 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 @@ -44,7 +44,7 @@ public class TestShutdownHookManager { /** * A new instance of ShutdownHookManager to ensure parallel tests - * don't have shared context + * don't have shared context. */ private final ShutdownHookManager mgr = new ShutdownHookManager(); @@ -224,10 +224,10 @@ public void testShutdownRemove() throws Throwable { Hook hook1 = new Hook("hook1", 0, false); Hook hook2 = new Hook("hook2", 0, false); mgr.addShutdownHook(hook1, 9); // create Hook1 with priority 9 - assertTrue(mgr.hasShutdownHook(hook1)); // hook1 lookup works + assertTrue("No hook1", mgr.hasShutdownHook(hook1)); // hook1 lookup works assertEquals(1, mgr.getShutdownHooksInOrder().size()); // 1 hook - assertFalse(mgr.removeShutdownHook(hook2)); // can't delete hook2 - assertTrue(mgr.removeShutdownHook(hook1)); // can delete hook1 + assertFalse("Delete hook2 should not be allowed", mgr.removeShutdownHook(hook2)); // can't delete hook2 + assertTrue("Can't delete hook1", mgr.removeShutdownHook(hook1)); // can delete hook1 assertEquals(0, mgr.getShutdownHooksInOrder().size()); // no more hooks } From f19165041e152afda7b6a8a322bfd70d96ebd347 Mon Sep 17 00:00:00 2001 From: Attila Magyar Date: Wed, 17 Jul 2019 14:25:33 +0200 Subject: [PATCH 4/4] HADOOP-16341. ShutDownHookManager: Regressed performance on Hook removals after HADOOP-15679 --- .../org/apache/hadoop/util/TestShutdownHookManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 ae916aa0c8b0e..59430e8bd9453 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.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import org.junit.After; import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -226,9 +225,10 @@ public void testShutdownRemove() throws Throwable { 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)); // can't delete hook2 - assertTrue("Can't delete hook1", mgr.removeShutdownHook(hook1)); // can delete hook1 - assertEquals(0, mgr.getShutdownHooksInOrder().size()); // no more hooks + 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();