Hive
  1. Hive
  2. HIVE-4500

HS2 holding too many file handles of hive_job_log_hive_*.txt files

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.11.0
    • Fix Version/s: 0.11.0
    • Component/s: HiveServer2
    • Labels:
      None

      Description

      In the hiveserver2 setup used for testing, we see that it has 2444 files open and of them 2152 are /tmp/hive/hive_job_log_hive_*.txt files

      1. HIVE-4500-3.patch
        5 kB
        Thejas M Nair
      2. HIVE-4500-2.patch
        5 kB
        Alan Gates
      3. HIVE-4500.patch
        5 kB
        Alan Gates

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hive-trunk-hadoop2 #189 (See https://builds.apache.org/job/Hive-trunk-hadoop2/189/)
          HIVE-4500 Ensure that HiveServer 2 closes log files. (Alan Gates via omalley) (Revision 1480390)

          Result = ABORTED
          omalley : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1480390
          Files :

          • /hive/trunk
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          • /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java
          • /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/Operation.java
          • /hive/trunk/service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
          Show
          Hudson added a comment - Integrated in Hive-trunk-hadoop2 #189 (See https://builds.apache.org/job/Hive-trunk-hadoop2/189/ ) HIVE-4500 Ensure that HiveServer 2 closes log files. (Alan Gates via omalley) (Revision 1480390) Result = ABORTED omalley : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1480390 Files : /hive/trunk /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/Operation.java /hive/trunk/service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
          Hide
          Hudson added a comment -

          Integrated in Hive-trunk-h0.21 #2093 (See https://builds.apache.org/job/Hive-trunk-h0.21/2093/)
          HIVE-4500 Ensure that HiveServer 2 closes log files. (Alan Gates via omalley) (Revision 1480390)

          Result = FAILURE
          omalley : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1480390
          Files :

          • /hive/trunk
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          • /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java
          • /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/Operation.java
          • /hive/trunk/service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
          Show
          Hudson added a comment - Integrated in Hive-trunk-h0.21 #2093 (See https://builds.apache.org/job/Hive-trunk-h0.21/2093/ ) HIVE-4500 Ensure that HiveServer 2 closes log files. (Alan Gates via omalley) (Revision 1480390) Result = FAILURE omalley : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1480390 Files : /hive/trunk /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/history/HiveHistory.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java /hive/trunk/service/src/java/org/apache/hive/service/cli/operation/Operation.java /hive/trunk/service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
          Hide
          Brock Noland added a comment -

          If the finally block calls a method like IOUtils.cleanup that discards exceptions it will miss exceptions on the close. For example, if the code looks like:

          That's an incorrect idiom and shouldn't damn all finally use.

          Regarding your example:

          OutputStream stream = null;
          try {
            ...
            stream.close();
          } catch (Throwable th) {
            IOUtils.cleanup(stream);
            throw new IOException("something", th);
          }
          

          Granted that was an example but a Throwable should never be blindly converted to an IOException. Hadoop is too often guilty of this. I believe the following to be more correct as it doesn't convert all Throwables to IOException and only eats an exception on close if a previous exception as thrown, which is the same as your example.

          OutputStream stream = null;
          try {
            ...
            stream.close();
            stream = null;
          } finally {
            IOUtils.cleanup(stream);
          }
          
          Show
          Brock Noland added a comment - If the finally block calls a method like IOUtils.cleanup that discards exceptions it will miss exceptions on the close. For example, if the code looks like: That's an incorrect idiom and shouldn't damn all finally use. Regarding your example: OutputStream stream = null ; try { ... stream.close(); } catch (Throwable th) { IOUtils.cleanup(stream); throw new IOException( "something" , th); } Granted that was an example but a Throwable should never be blindly converted to an IOException. Hadoop is too often guilty of this. I believe the following to be more correct as it doesn't convert all Throwables to IOException and only eats an exception on close if a previous exception as thrown, which is the same as your example. OutputStream stream = null ; try { ... stream.close(); stream = null ; } finally { IOUtils.cleanup(stream); }
          Hide
          Owen O'Malley added a comment -

          I just committed this to branch-0.11 and trunk. Thanks, Alan!

          Show
          Owen O'Malley added a comment - I just committed this to branch-0.11 and trunk. Thanks, Alan!
          Hide
          Owen O'Malley added a comment -

          Which part of the patch are you referring to ? Is it the change in HiveSessionImpl.java ?

          I'm sorry, I was assuming that since you were calling IOUtils.cleanup, which throws away exceptions, that it was in a finally block outside of the patch.

          This is not true as long as the finally block does not throw an exception of it's own. Thus finally blocks should not throw an Exception.

          If the finally block calls a method like IOUtils.cleanup that discards exceptions it will miss exceptions on the close. For example, if the code looks like:

          OutputStream stream = null;
          try {
            ...
          } finally {
            IOUtils.cleanup(stream);
          }
          

          Then in the case where the close, which likely includes the last write to the stream, fails that exception will be lost. The right code is:

          OutputStream stream = null;
          try {
            ...
            stream.close();
          } catch (Throwable th) {
            IOUtils.cleanup(stream);
            throw new IOException("something", th);
          }
          
          Show
          Owen O'Malley added a comment - Which part of the patch are you referring to ? Is it the change in HiveSessionImpl.java ? I'm sorry, I was assuming that since you were calling IOUtils.cleanup, which throws away exceptions, that it was in a finally block outside of the patch. This is not true as long as the finally block does not throw an exception of it's own. Thus finally blocks should not throw an Exception. If the finally block calls a method like IOUtils.cleanup that discards exceptions it will miss exceptions on the close. For example, if the code looks like: OutputStream stream = null ; try { ... } finally { IOUtils.cleanup(stream); } Then in the case where the close, which likely includes the last write to the stream, fails that exception will be lost. The right code is: OutputStream stream = null ; try { ... stream.close(); } catch (Throwable th) { IOUtils.cleanup(stream); throw new IOException( "something" , th); }
          Hide
          Brock Noland added a comment -

          I dislike the use of try finally in Java, because it either causes the code to ignore exceptions in the normal case or hide exceptions in the exception case.

          This is not true as long as the finally block does not throw an exception of it's own. Thus finally blocks should not throw an Exception.

          Show
          Brock Noland added a comment - I dislike the use of try finally in Java, because it either causes the code to ignore exceptions in the normal case or hide exceptions in the exception case. This is not true as long as the finally block does not throw an exception of it's own. Thus finally blocks should not throw an Exception.
          Hide
          Thejas M Nair added a comment -

          I dislike the use of try finally in Java

          Which part of the patch are you referring to ? Is it the change in HiveSessionImpl.java ?

          Show
          Thejas M Nair added a comment - I dislike the use of try finally in Java Which part of the patch are you referring to ? Is it the change in HiveSessionImpl.java ?
          Hide
          Owen O'Malley added a comment -

          I started the tests on this last night and will commit it if they pass.

          I dislike the use of try finally in Java, because it either causes the code to ignore exceptions in the normal case or hide exceptions in the exception case. Obviously this patch is taking the ignore exceptions option. We should fix it, but not when the fix is on the critical path.

          Show
          Owen O'Malley added a comment - I started the tests on this last night and will commit it if they pass. I dislike the use of try finally in Java, because it either causes the code to ignore exceptions in the normal case or hide exceptions in the exception case. Obviously this patch is taking the ignore exceptions option. We should fix it, but not when the fix is on the critical path.
          Hide
          Thejas M Nair added a comment -

          I am unable to upload the new patch to RB link that Alan created, so I created a new one - https://reviews.apache.org/r/10986/
          I have uploaded patch 1 and patch 3 to it so its easy to look at the changes between them. (RB was giving me trouble uploading patch2 for some reason!)

          Show
          Thejas M Nair added a comment - I am unable to upload the new patch to RB link that Alan created, so I created a new one - https://reviews.apache.org/r/10986/ I have uploaded patch 1 and patch 3 to it so its easy to look at the changes between them. (RB was giving me trouble uploading patch2 for some reason!)
          Hide
          Thejas M Nair added a comment -

          HIVE-4500-3.patch -
          Setting hive.session.history.enabled to false results in NPE in several places. This is because code calling SessionState.getHiveHistory() does not check for nulls.

          I am removing this config param change from the patch, so that we can go ahead with rest of the changes for hive 0.11 rc

          I will make the parameter part of HIVE-4513 that I am working on.

          Show
          Thejas M Nair added a comment - HIVE-4500 -3.patch - Setting hive.session.history.enabled to false results in NPE in several places. This is because code calling SessionState.getHiveHistory() does not check for nulls. I am removing this config param change from the patch, so that we can go ahead with rest of the changes for hive 0.11 rc I will make the parameter part of HIVE-4513 that I am working on.
          Hide
          Carl Steinbach added a comment -

          +1

          @Alan: Changes look good. I forgot to mention that we also add new properties to conf/hive-default.xml.template with a short description for documentation purposes. It would be great if you're willing to update this, but I'm fine with committing this as-is. Either way, can someone at HW test this on your build farm and let me know if it passes? Thanks.

          Show
          Carl Steinbach added a comment - +1 @Alan: Changes look good. I forgot to mention that we also add new properties to conf/hive-default.xml.template with a short description for documentation purposes. It would be great if you're willing to update this, but I'm fine with committing this as-is. Either way, can someone at HW test this on your build farm and let me know if it passes? Thanks.
          Hide
          Alan Gates added a comment -

          Addressed Carl's comments.

          Show
          Alan Gates added a comment - Addressed Carl's comments.
          Hide
          Carl Steinbach added a comment -

          I left comments on rb. Thanks.

          Show
          Carl Steinbach added a comment - I left comments on rb. Thanks.
          Hide
          Alan Gates added a comment -
          Show
          Alan Gates added a comment - Review request at https://reviews.apache.org/r/10954/
          Hide
          Carl Steinbach added a comment -

          @Alan: I have some comments. Can you please create a review request?

          Show
          Carl Steinbach added a comment - @Alan: I have some comments. Can you please create a review request?

            People

            • Assignee:
              Alan Gates
              Reporter:
              Alan Gates
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development