Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1250

Refactor job token to use a common token interface

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The idea is to use a common token interface for both job token and delegation token (HADOOP-6373) so that the RPC layer that uses them don't have to differentiate them.

      1. m1250-09.patch
        36 kB
        Kan Zhang
      2. m1250-12.patch
        36 kB
        Kan Zhang
      3. m1250-14.patch
        36 kB
        Kan Zhang
      4. MR-1250-0_20.2.patch
        34 kB
        Jitendra Nath Pandey

        Issue Links

          Activity

          Hide
          Kan Zhang added a comment -

          Attached a patch that re-factored existing job token code to use the new interface being added in HADOOP-6415. This patch is derived from earlier discussions in HADOOP-6373.

          Show
          Kan Zhang added a comment - Attached a patch that re-factored existing job token code to use the new interface being added in HADOOP-6415 . This patch is derived from earlier discussions in HADOOP-6373 .
          Hide
          Owen O'Malley added a comment -

          A few comments:

          1. All of the JobToken classes should be marked @Audience.Private
          2. I think it may make more sense to store a Token in the Task, especially since it is Writable and can be easily serialized as part of the Task's write method.
          Show
          Owen O'Malley added a comment - A few comments: All of the JobToken classes should be marked @Audience.Private I think it may make more sense to store a Token in the Task, especially since it is Writable and can be easily serialized as part of the Task's write method.
          Hide
          Kan Zhang added a comment -

          > I think it may make more sense to store a Token in the Task, especially since it is Writable and can be easily serialized as part of the Task's write method.

          Currently, it only servers as a temporary in-memory cache for the SecretKey (to avoid converting from tokenPassword to SecretKey each time the token is used for Shuffle). The token itself is not intended to be serialized and sent along with the Task object. The passing of credentials for a Task is handled by way of the credential cache. If we're going to pass credentials along with Task objects, we need to make sure Task objects are handled properly. Since this is a re-factoring patch, I suggest we evaluate it as part of the credential cache work Boris is doing.

          Attaching a patch that addressed your other comments.

          Show
          Kan Zhang added a comment - > I think it may make more sense to store a Token in the Task, especially since it is Writable and can be easily serialized as part of the Task's write method. Currently, it only servers as a temporary in-memory cache for the SecretKey (to avoid converting from tokenPassword to SecretKey each time the token is used for Shuffle). The token itself is not intended to be serialized and sent along with the Task object. The passing of credentials for a Task is handled by way of the credential cache. If we're going to pass credentials along with Task objects, we need to make sure Task objects are handled properly. Since this is a re-factoring patch, I suggest we evaluate it as part of the credential cache work Boris is doing. Attaching a patch that addressed your other comments.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12428336/m1250-12.patch
          against trunk revision 892411.

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

          +1 tests included. The patch appears to include 6 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 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/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/12428336/m1250-12.patch against trunk revision 892411. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/221/console This message is automatically generated.
          Hide
          Devaraj Das added a comment -

          This is looking good except for one spurious comment "// storage of the job token secret" in Task.java, and a white line change in TaskTracker.java.
          Regarding sending the token with the task object or not, in the future (HADOOP-6325), there would optionally be a credential cache that is passed to the JobTracker during job submission. This cache has to be persisted to support job recovery. I am thinking that we could add the JobTracker generated JobToken to the same file. The current approach in the patch is in line with that..

          Show
          Devaraj Das added a comment - This is looking good except for one spurious comment "// storage of the job token secret" in Task.java, and a white line change in TaskTracker.java. Regarding sending the token with the task object or not, in the future ( HADOOP-6325 ), there would optionally be a credential cache that is passed to the JobTracker during job submission. This cache has to be persisted to support job recovery. I am thinking that we could add the JobTracker generated JobToken to the same file. The current approach in the patch is in line with that..
          Hide
          Kan Zhang added a comment -

          Attached a patch that addressed Devaraj's comments.
          > I am thinking that we could add the JobTracker generated JobToken to the same file.
          Ditto.

          Show
          Kan Zhang added a comment - Attached a patch that addressed Devaraj's comments. > I am thinking that we could add the JobTracker generated JobToken to the same file. Ditto.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12428646/m1250-14.patch
          against trunk revision 892893.

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

          +1 tests included. The patch appears to include 6 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 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/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/12428646/m1250-14.patch against trunk revision 892893. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/328/console This message is automatically generated.
          Hide
          Devaraj Das added a comment -

          The core test failures are unrelated. They seem to be due to MAPREDUCE-1275. The Streaming tests failures are known issues.

          Show
          Devaraj Das added a comment - The core test failures are unrelated. They seem to be due to MAPREDUCE-1275 . The Streaming tests failures are known issues.
          Hide
          Devaraj Das added a comment -

          Forgot to mention that I committed the patch. Thanks, Kan!

          Show
          Devaraj Das added a comment - Forgot to mention that I committed the patch. Thanks, Kan!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #196 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/196/)

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #196 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/196/ )
          Hide
          Jitendra Nath Pandey added a comment -

          Patch for Hadoop 20 added.

          Show
          Jitendra Nath Pandey added a comment - Patch for Hadoop 20 added.

            People

            • Assignee:
              Kan Zhang
              Reporter:
              Kan Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development