Hadoop Common
  1. Hadoop Common
  2. HADOOP-4829

Allow FileSystem shutdown hook to be disabled

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.18.1
    • Fix Version/s: 0.21.0
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      New configuration parameter fs.automatic.close can be set false to disable the JVM shutdown hook that automatically closes FileSystems.

      Description

      FileSystem sets a JVM shutdown hook so that it can clean up the FileSystem cache. This is great behavior when you are writing a client application, but when you're writing a server application, like the Collector or an HBase RegionServer, you need to control the shutdown of the application and HDFS much more closely. If you set your own shutdown hook, there's no guarantee that your hook will run before the HDFS one, preventing you from taking some shutdown actions.

      The current workaround I've used is to snag the FileSystem shutdown hook via Java reflection, disable it, and then run it on my own schedule. I'd really appreciate not having to do take this hacky approach. It seems like the right way to go about this is to just to add a method to disable the hook directly on FileSystem. That way, server applications can elect to disable the automatic cleanup and just call FileSystem.closeAll themselves when the time is right.

      1. hadoop-4829-v3.txt
        7 kB
        Todd Lipcon
      2. hadoop-4829-v2.txt
        7 kB
        Todd Lipcon
      3. HADOOP-4829-0.18.3.patch
        7 kB
        Todd Lipcon
      4. hadoop-4829.txt
        7 kB
        Todd Lipcon

        Activity

        Hide
        Jim Kellerman added a comment -

        +1

        Show
        Jim Kellerman added a comment - +1
        Hide
        Stefan Will added a comment -

        +1

        Show
        Stefan Will added a comment - +1
        Hide
        Todd Lipcon added a comment -

        Would a new configuration boolean 'fs.manual.shutdown' be adequate for your needs? You could programatically set this before getting your filesystems, and when set true, the shutdown hook would not be added.

        Show
        Todd Lipcon added a comment - Would a new configuration boolean 'fs.manual.shutdown' be adequate for your needs? You could programatically set this before getting your filesystems, and when set true, the shutdown hook would not be added.
        Hide
        Bryan Duxbury added a comment -

        That would be great.

        Show
        Bryan Duxbury added a comment - That would be great.
        Hide
        Jim Kellerman added a comment -

        The configuration boolean would be just fine.

        Show
        Jim Kellerman added a comment - The configuration boolean would be just fine.
        Hide
        Todd Lipcon added a comment -

        This patch introduces a new configuration variable fs.automatic.shutdown defaulted to true which prevents the JVM shutdown hook from closing the filesystem. This variable is read on a per-FS basis when the FS is added to the FileSystem.Cache.

        As a side effect of this patch, I refactored the ClientFinalizer thread inside the Cache class.

        The new test case uses a FS implementation that reports when its close() method has been fired to ensure that the new logic works correctly.

        Show
        Todd Lipcon added a comment - This patch introduces a new configuration variable fs.automatic.shutdown defaulted to true which prevents the JVM shutdown hook from closing the filesystem. This variable is read on a per-FS basis when the FS is added to the FileSystem.Cache. As a side effect of this patch, I refactored the ClientFinalizer thread inside the Cache class. The new test case uses a FS implementation that reports when its close() method has been fired to ensure that the new logic works correctly.
        Hide
        Bryan Duxbury added a comment -

        I think the patch looks good. +1.

        Show
        Bryan Duxbury added a comment - I think the patch looks good. +1.
        Hide
        Tom White added a comment -

        +1 Looks good.

        One minor nit: Should the naming use the term "close" rather than "shutdown" for consistency with the method name on FileSystem? For example, call the property "fs.automatic.close", and in the description of the property say: "By default, FileSystem instances are automatically closed at program exit using a JVM shutdown hook."

        Show
        Tom White added a comment - +1 Looks good. One minor nit: Should the naming use the term "close" rather than "shutdown" for consistency with the method name on FileSystem? For example, call the property "fs.automatic.close", and in the description of the property say: "By default, FileSystem instances are automatically closed at program exit using a JVM shutdown hook."
        Hide
        Todd Lipcon added a comment -

        Thanks for the review, Tom. Here's an updated version with that change.

        Show
        Todd Lipcon added a comment - Thanks for the review, Tom. Here's an updated version with that change.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12407279/hadoop-4829-v2.txt
        against trunk revision 772059.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12407279/hadoop-4829-v2.txt against trunk revision 772059. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/294/console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Contrib test failures appear to be some unrelated problem on the hudson machine

        Show
        Todd Lipcon added a comment - Contrib test failures appear to be some unrelated problem on the hudson machine
        Hide
        Todd Lipcon added a comment -

        Toggling patch status to retrigger hudson

        Show
        Todd Lipcon added a comment - Toggling patch status to retrigger hudson
        Hide
        Tom White added a comment -

        Todd.

        This patch no longer applies following the changes to the test source tree. Could you please generate a new version?

        Show
        Tom White added a comment - Todd. This patch no longer applies following the changes to the test source tree. Could you please generate a new version?
        Hide
        Todd Lipcon added a comment -

        Here's a patch against branch 18 if anyone wants this feature backported. However, it probably does not need to be committed to the Apache 18 branch since it's a new feature rather than a fix.

        Show
        Todd Lipcon added a comment - Here's a patch against branch 18 if anyone wants this feature backported. However, it probably does not need to be committed to the Apache 18 branch since it's a new feature rather than a fix.
        Hide
        Todd Lipcon added a comment -

        Rebased this patch against current trunk (old patch didn't apply because of a test dir rename)

        Show
        Todd Lipcon added a comment - Rebased this patch against current trunk (old patch didn't apply because of a test dir rename)
        Hide
        Tom White added a comment -

        I've just committed this. Thanks Todd! (Could you fill in the release note please.)

        Show
        Tom White added a comment - I've just committed this. Thanks Todd! (Could you fill in the release note please.)
        Hide
        Robert Chansler added a comment -

        Editorial pass over all release notes prior to publication of 0.21.

        Show
        Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21.
        Hide
        Ted Yu added a comment -

        I saw the following after applying this patch:
        Exception in thread "Processor Shutdown Hook" java.lang.IllegalStateException: Shutdown in progress
        at java.lang.ApplicationShutdownHooks.remove(ApplicationShutdownHooks.java:55)
        at java.lang.Runtime.removeShutdownHook(Runtime.java:220)
        at org.apache.hadoop.fs.FileSystem$Cache.remove(FileSystem.java:1408)
        at org.apache.hadoop.fs.FileSystem$Cache.closeAll(FileSystem.java:1440)
        at org.apache.hadoop.fs.FileSystem$Cache.closeAll(FileSystem.java:1417)
        at org.apache.hadoop.fs.FileSystem.closeAll(FileSystem.java:202)

        Here is the modified Cache.remove() method where IllegalStateException is handled:
        synchronized void remove(Key key, FileSystem fs) {
        if (map.containsKey(key) && fs == map.get(key)) {
        map.remove(key);
        toAutoClose.remove(key);
        if (map.isEmpty() && !clientFinalizer.isAlive()) {
        try
        {
        if (!Runtime.getRuntime().removeShutdownHook(clientFinalizer))

        { LOG.info("Could not cancel cleanup thread, though no " + "FileSystems are open"); }

        }
        // ignore IllegalStateException because Shutdown in progress
        catch (java.lang.IllegalStateException ise)
        {
        }
        }
        }
        }

        Show
        Ted Yu added a comment - I saw the following after applying this patch: Exception in thread "Processor Shutdown Hook" java.lang.IllegalStateException: Shutdown in progress at java.lang.ApplicationShutdownHooks.remove(ApplicationShutdownHooks.java:55) at java.lang.Runtime.removeShutdownHook(Runtime.java:220) at org.apache.hadoop.fs.FileSystem$Cache.remove(FileSystem.java:1408) at org.apache.hadoop.fs.FileSystem$Cache.closeAll(FileSystem.java:1440) at org.apache.hadoop.fs.FileSystem$Cache.closeAll(FileSystem.java:1417) at org.apache.hadoop.fs.FileSystem.closeAll(FileSystem.java:202) Here is the modified Cache.remove() method where IllegalStateException is handled: synchronized void remove(Key key, FileSystem fs) { if (map.containsKey(key) && fs == map.get(key)) { map.remove(key); toAutoClose.remove(key); if (map.isEmpty() && !clientFinalizer.isAlive()) { try { if (!Runtime.getRuntime().removeShutdownHook(clientFinalizer)) { LOG.info("Could not cancel cleanup thread, though no " + "FileSystems are open"); } } // ignore IllegalStateException because Shutdown in progress catch (java.lang.IllegalStateException ise) { } } } }
        Hide
        Ted Yu added a comment -

        Clarification:
        The comment about Cache.remove() was made for patched 0.20.1.

        Show
        Ted Yu added a comment - Clarification: The comment about Cache.remove() was made for patched 0.20.1.

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Bryan Duxbury
          • Votes:
            2 Vote for this issue
            Watchers:
            13 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development