Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.1, 0.24.0
    • Fix Version/s: 0.23.2
    • Component/s: util
    • Labels:
      None

      Description

      There is an existing Credentials#addAll which combines two Credentials, but it overwrites all existing Credentials. There should be a Credentials#mergeAll that will not overwrite. This will facility the cleanup of code in TokenCache denoted with:

      //TODO: Need to come up with a better place to put
      //this block of code to do with reading the file
      

      The token cache basically needs to merge the contents of a binary credentials file when it fails to find a token. Performing the merge within Credentials is cleaner, and will break the cross-component dependency whereby the TokenCache currently has to have intimate knowledge of how the FileSystem will key tokens in the cache.

      1. HADOOP-8048-2.patch
        5 kB
        Daryn Sharp
      2. HADOOP-8048.patch
        4 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Hadoop QA added a comment -

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

          +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 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 .

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/579//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/579//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/12513976/HADOOP-8048.patch against trunk revision . +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 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 . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/579//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/579//console This message is automatically generated.
          Hide
          Thomas Graves added a comment -

          overall looks good. A couple minor nits.

          • it would be nice to have another token and service already in creds object and verify that they aren't changed/removed during the addAll/mergeAll.
          • very minor - in the test for mergeAll I think your comments about check for new/existing don't match up with the asserts exactly. its checks 2 tokens, then 2 services and it should be 1 token/1 service and then 1 token/1service. see addAll for the right order.
          Show
          Thomas Graves added a comment - overall looks good. A couple minor nits. it would be nice to have another token and service already in creds object and verify that they aren't changed/removed during the addAll/mergeAll. very minor - in the test for mergeAll I think your comments about check for new/existing don't match up with the asserts exactly. its checks 2 tokens, then 2 services and it should be 1 token/1 service and then 1 token/1service. see addAll for the right order.
          Hide
          Thomas Graves added a comment -

          Sorry to clarify the first comment - in the tests for addAll and mergeAll. I think it would be useful to add another token and service that won't have a corresponding one in the credsToAdd, and then assert after doing the addAll/mergeAll that that token and service still exist and haven't changed.

          Show
          Thomas Graves added a comment - Sorry to clarify the first comment - in the tests for addAll and mergeAll. I think it would be useful to add another token and service that won't have a corresponding one in the credsToAdd, and then assert after doing the addAll/mergeAll that that token and service still exist and haven't changed.
          Hide
          Daryn Sharp added a comment -

          Update tests per Tom.

          Show
          Daryn Sharp added a comment - Update tests per Tom.
          Hide
          Hadoop QA added a comment -

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

          +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 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 .

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/583//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/583//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/12514021/HADOOP-8048-2.patch against trunk revision . +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 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 . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/583//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/583//console This message is automatically generated.
          Hide
          Thomas Graves added a comment -

          +1 lgtm.

          Show
          Thomas Graves added a comment - +1 lgtm.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #1781 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1781/)
          HADOOP-8048. Allow merging of Credentials (Daryn Sharp via tgraves)

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1781 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1781/ ) HADOOP-8048 . Allow merging of Credentials (Daryn Sharp via tgraves) tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1706 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1706/)
          HADOOP-8048. Allow merging of Credentials (Daryn Sharp via tgraves)

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1706 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1706/ ) HADOOP-8048 . Allow merging of Credentials (Daryn Sharp via tgraves) tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1717 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1717/)
          HADOOP-8048. Allow merging of Credentials (Daryn Sharp via tgraves)

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1717 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1717/ ) HADOOP-8048 . Allow merging of Credentials (Daryn Sharp via tgraves) tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Thomas Graves added a comment -

          Thanks Daryn! I committed this to trunk and branch 0.23.

          Show
          Thomas Graves added a comment - Thanks Daryn! I committed this to trunk and branch 0.23.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Commit #522 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/522/)
          merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Commit #522 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/522/ ) merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048 tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-0.23-Commit #533 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/533/)
          merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-0.23-Commit #533 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/533/ ) merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048 tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-0.23-Commit #537 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/537/)
          merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Commit #537 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/537/ ) merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048 tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #952 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/952/)
          HADOOP-8048. Allow merging of Credentials (Daryn Sharp via tgraves)

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #952 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/952/ ) HADOOP-8048 . Allow merging of Credentials (Daryn Sharp via tgraves) tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Build #165 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/165/)
          merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #165 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/165/ ) merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048 tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-0.23-Build #187 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/187/)
          merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Build #187 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Build/187/ ) merge -r 1242615:1242616 from trunk. FIXES: HADOOP-8048 tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242622 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #985 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/985/)
          HADOOP-8048. Allow merging of Credentials (Daryn Sharp via tgraves)

          tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #985 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/985/ ) HADOOP-8048 . Allow merging of Credentials (Daryn Sharp via tgraves) tgraves : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1242616 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Daryn Sharp
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development