HADOOP-16341. ShutDownHookManager: Regressed performance on Hook removals after HADOOP-15679

Contributed by Gopal V and Atilla Magyar.

Change-Id: I066d5eece332a1673594de0f9b484443f95530ec

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());
@@ -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<HookEntry> getShutdownHooksInOrder() {
     List<HookEntry> list;
-    synchronized (MGR.hooks) {
-      list = new ArrayList<>(MGR.hooks);
+    synchronized (hooks) {
+      list = new ArrayList<>(hooks);
     }
     Collections.sort(list, new Comparator<HookEntry>() {
 
@@ -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));
   }
   
   /**

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;
@@ -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 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);
@@ -119,7 +114,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);
 
@@ -193,7 +188,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 +216,21 @@ 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("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();
 
   /**