Hadoop Common
  1. Hadoop Common
  2. HADOOP-10541

InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 2.4.0
    • Fix Version/s: 2.5.0
    • Component/s: test
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      The same InputStream variable is used for minikdc.ldiff and minikdc-krb5.conf :

          InputStream is = cl.getResourceAsStream("minikdc.ldiff");
      ...
          is = cl.getResourceAsStream("minikdc-krb5.conf");
      

      Before the second assignment, is should be closed.

      1. HADOOP-10541.1.patch.txt
        2 kB
        Swarnim Kulkarni
      2. HADOOP-10541.2.patch.txt
        3 kB
        Swarnim Kulkarni
      3. HADOOP-10541.3.patch.txt
        3 kB
        Swarnim Kulkarni
      4. HADOOP-10541.4.patch
        3 kB
        Chris Nauroth

        Activity

        Ted Yu created issue -
        Hide
        Swarnim Kulkarni added a comment -

        Ted Yu If you are ok with it, I'll like to take a stab at this.

        Show
        Swarnim Kulkarni added a comment - Ted Yu If you are ok with it, I'll like to take a stab at this.
        Hide
        Swarnim Kulkarni added a comment -

        Patch attached.

        Show
        Swarnim Kulkarni added a comment - Patch attached.
        Swarnim Kulkarni made changes -
        Field Original Value New Value
        Attachment HADOOP-10541.1.patch.txt [ 12642996 ]
        Swarnim Kulkarni made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12642996/HADOOP-10541.1.patch.txt
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-minikdc.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3888//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3888//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/12642996/HADOOP-10541.1.patch.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-minikdc. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3888//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3888//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Thanks for the patch.

        Show
        Ted Yu added a comment - Thanks for the patch.
        Hide
        Swarnim Kulkarni added a comment -

        This should be ready for review.

        Show
        Swarnim Kulkarni added a comment - This should be ready for review.
        Chris Nauroth made changes -
        Assignee Swarnim Kulkarni [ swarnim ]
        Hide
        Chris Nauroth added a comment -

        Hi, Swarnim Kulkarni. Thank you for providing the patch.

        I still see a few opportunities for a resource leak in this code. For is1, there is a call to IOUtils#toString outside the try block. This can throw IOException, which would then result in is1 not being closed. For is2, there is a similar situation with BufferedReader#readLine, which can throw IOException. In both cases, I recommend moving the calls that can throw inside the try block. These are existing issues that weren't introduced by your new patch, but it would be convenient to clean them up now, since you're changing this code path anyway.

        Also, the standard indentation for the project is to indent by 2 spaces (no tabs). I saw a few tabs in your patch, so would you please convert those to 2 spaces?

        Thanks again!

        Show
        Chris Nauroth added a comment - Hi, Swarnim Kulkarni . Thank you for providing the patch. I still see a few opportunities for a resource leak in this code. For is1 , there is a call to IOUtils#toString outside the try block. This can throw IOException , which would then result in is1 not being closed. For is2 , there is a similar situation with BufferedReader#readLine , which can throw IOException . In both cases, I recommend moving the calls that can throw inside the try block. These are existing issues that weren't introduced by your new patch, but it would be convenient to clean them up now, since you're changing this code path anyway. Also, the standard indentation for the project is to indent by 2 spaces (no tabs). I saw a few tabs in your patch, so would you please convert those to 2 spaces? Thanks again!
        Chris Nauroth made changes -
        Affects Version/s 2.4.0 [ 12326144 ]
        Affects Version/s 3.0.0 [ 12320357 ]
        Target Version/s 3.0.0, 2.5.0 [ 12320357, 12326263 ]
        Component/s test [ 12311440 ]
        Hide
        Swarnim Kulkarni added a comment -

        Thanks for the feedback Chris. I addressed your concerns and attached a new patch. Let me know if this looks any better. Thanks.

        Show
        Swarnim Kulkarni added a comment - Thanks for the feedback Chris. I addressed your concerns and attached a new patch. Let me know if this looks any better. Thanks.
        Swarnim Kulkarni made changes -
        Attachment HADOOP-10541.2.patch.txt [ 12643251 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12643251/HADOOP-10541.2.patch.txt
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-minikdc.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3907//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3907//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/12643251/HADOOP-10541.2.patch.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-minikdc. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3907//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3907//console This message is automatically generated.
        Hide
        Swarnim Kulkarni added a comment -

        New patch attached with minor cleanup.

        Show
        Swarnim Kulkarni added a comment - New patch attached with minor cleanup.
        Swarnim Kulkarni made changes -
        Attachment HADOOP-10541.3.patch.txt [ 12643318 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12643318/HADOOP-10541.3.patch.txt
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-minikdc.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3911//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3911//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/12643318/HADOOP-10541.3.patch.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-minikdc. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3911//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3911//console This message is automatically generated.
        Hide
        Chris Nauroth added a comment -

        +1 for the patch. Thank you for addressing the feedback. I'll commit this.

        Show
        Chris Nauroth added a comment - +1 for the patch. Thank you for addressing the feedback. I'll commit this.
        Chris Nauroth made changes -
        Hadoop Flags Reviewed [ 10343 ]
        Hide
        Chris Nauroth added a comment -

        Actually, I spotted one more minor indentation issue. Rather than go back and forth again, I just corrected it locally and generated a new patch. I'm uploading v4. I'll commit this version after Jenkins tries it.

        Show
        Chris Nauroth added a comment - Actually, I spotted one more minor indentation issue. Rather than go back and forth again, I just corrected it locally and generated a new patch. I'm uploading v4. I'll commit this version after Jenkins tries it.
        Chris Nauroth made changes -
        Attachment HADOOP-10541.4.patch [ 12643409 ]
        Hide
        Swarnim Kulkarni added a comment -

        Thanks Chris and sorry for that. Is there an eclipse formatter that I can use for my future contributions? I tried hunting one here [1] but I couldn't find one. Only one I found was here[2]. Is it ok to use that?

        Thanks again.

        [1] http://wiki.apache.org/hadoop/EclipseEnvironment
        [2] https://github.com/cloudera/blog-eclipse/blob/master/hadoop-format.xml

        Show
        Swarnim Kulkarni added a comment - Thanks Chris and sorry for that. Is there an eclipse formatter that I can use for my future contributions? I tried hunting one here [1] but I couldn't find one. Only one I found was here [2] . Is it ok to use that? Thanks again. [1] http://wiki.apache.org/hadoop/EclipseEnvironment [2] https://github.com/cloudera/blog-eclipse/blob/master/hadoop-format.xml
        Hide
        Chris Nauroth added a comment -

        That's no problem. Thank you for contributing a patch. These little nitpick things are easy to address in review.

        Unfortunately, I can't help you with advice on an Eclipse formatter, because I don't use Eclipse. Maybe another community member who does use Eclipse will chime in. We also have this page:

        https://wiki.apache.org/hadoop/CodeReviewChecklist

        Our stated standard is "follows Sun's code conventions except indentation is 2 spaces, not 4". Maybe it's sufficient simply to clone the default formatting profile you get with Eclipse and then switch the indentation to 2 spaces/no tabs. I seem to recall from my Eclipse days that there are 2 places you need to configure this: one setting for the number of spaces and another setting for using space characters instead of tab characters.

        Show
        Chris Nauroth added a comment - That's no problem. Thank you for contributing a patch. These little nitpick things are easy to address in review. Unfortunately, I can't help you with advice on an Eclipse formatter, because I don't use Eclipse. Maybe another community member who does use Eclipse will chime in. We also have this page: https://wiki.apache.org/hadoop/CodeReviewChecklist Our stated standard is "follows Sun's code conventions except indentation is 2 spaces, not 4". Maybe it's sufficient simply to clone the default formatting profile you get with Eclipse and then switch the indentation to 2 spaces/no tabs. I seem to recall from my Eclipse days that there are 2 places you need to configure this: one setting for the number of spaces and another setting for using space characters instead of tab characters.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12643409/HADOOP-10541.4.patch
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-minikdc.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3914//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3914//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/12643409/HADOOP-10541.4.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-minikdc. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3914//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3914//console This message is automatically generated.
        Hide
        Chris Nauroth added a comment -

        I committed this to trunk and branch-2. Swarnim, thank you again for contributing the patch.

        Show
        Chris Nauroth added a comment - I committed this to trunk and branch-2. Swarnim, thank you again for contributing the patch.
        Chris Nauroth made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Fix Version/s 3.0.0 [ 12320357 ]
        Fix Version/s 2.5.0 [ 12326263 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hadoop-trunk-Commit #5594 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5594/)
        HADOOP-10541. InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5594 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5594/ ) HADOOP-10541 . InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Yarn-trunk #559 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/559/)
        HADOOP-10541. InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #559 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/559/ ) HADOOP-10541 . InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #1751 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1751/)
        HADOOP-10541. InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Show
        Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1751 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1751/ ) HADOOP-10541 . InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #1777 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1777/)
        HADOOP-10541. InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Show
        Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1777 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1777/ ) HADOOP-10541 . InputStream in MiniKdc#initKDCServer for minikdc.ldiff is not closed. Contributed by Swarnim Kulkarni. (cnauroth: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1592803 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-minikdc/src/main/java/org/apache/hadoop/minikdc/MiniKdc.java
        Karthik Kambatla (Inactive) made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Allen Wittenauer made changes -
        Fix Version/s 3.0.0 [ 12320357 ]

          People

          • Assignee:
            Swarnim Kulkarni
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development