Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: mrv2
    • Labels:
      None

      Description

      We are directly accessing MapTaskImpl in TaskImpl.InitialScheduleTransition.transition(..). It'll be better to reorganize the code so each subclass can provide its own behavior instead of explicitly checking for the subclass type.

      1. MAPREDUCE-2661_rev2.patch
        4 kB
        Ahmed Radwan
      2. MAPREDUCE-2661_rev3.patch
        3 kB
        Ahmed Radwan
      3. MAPREDUCE-2661.patch
        4 kB
        Ahmed Radwan

        Activity

        Hide
        Ahmed Radwan added a comment -

        Reorganization of the related code in TaskImpl, MapTaskImpl and ReduceTaskImpl.

        Show
        Ahmed Radwan added a comment - Reorganization of the related code in TaskImpl, MapTaskImpl and ReduceTaskImpl.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12485693/MAPREDUCE-2661.patch
        against trunk revision 1144097.

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/443//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/12485693/MAPREDUCE-2661.patch against trunk revision 1144097. +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/443//console This message is automatically generated.
        Hide
        Josh Wills added a comment -

        +1

        An optional thought: add a comment to the definition of the getSplitsAsString() method in TaskImpl.java to specify that it will return the empty string for reduce tasks since they don't have splits.

        Show
        Josh Wills added a comment - +1 An optional thought: add a comment to the definition of the getSplitsAsString() method in TaskImpl.java to specify that it will return the empty string for reduce tasks since they don't have splits.
        Hide
        Ahmed Radwan added a comment -

        Thanks Josh for your review! I have updated the patch to include comments for the added methods.

        Show
        Ahmed Radwan added a comment - Thanks Josh for your review! I have updated the patch to include comments for the added methods.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12485772/MAPREDUCE-2661_rev2.patch
        against trunk revision 1144403.

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/449//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/12485772/MAPREDUCE-2661_rev2.patch against trunk revision 1144403. +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/449//console This message is automatically generated.
        Hide
        Devaraj K added a comment -

        I feel it is not correct to give the getSplitsAsString() implementation details of MapTaskImpl.java and ReduceTaskImpl.java in TaskImpl.java.

        TaskImpl.java can provide default implementation of getSplitsAsString() instead of making it as abstract and returning with empty string in ReduceTaskImpl.java.

        Show
        Devaraj K added a comment - I feel it is not correct to give the getSplitsAsString() implementation details of MapTaskImpl.java and ReduceTaskImpl.java in TaskImpl.java. TaskImpl.java can provide default implementation of getSplitsAsString() instead of making it as abstract and returning with empty string in ReduceTaskImpl.java.
        Hide
        Ahmed Radwan added a comment -

        Thanks Devaraj for your review!

        > I feel it is not correct to give the getSplitsAsString() implementation details of MapTaskImpl.java and ReduceTaskImpl.java in TaskImpl.java.

        Yes, and this is why the patch pushes the implementation detail to subclasses.

        > TaskImpl.java can provide default implementation of getSplitsAsString() instead of making it as abstract and returning with empty string in ReduceTaskImpl.java.

        Agree, it will be similar that way. I have updated the patch accordingly.

        Thanks

        Show
        Ahmed Radwan added a comment - Thanks Devaraj for your review! > I feel it is not correct to give the getSplitsAsString() implementation details of MapTaskImpl.java and ReduceTaskImpl.java in TaskImpl.java. Yes, and this is why the patch pushes the implementation detail to subclasses. > TaskImpl.java can provide default implementation of getSplitsAsString() instead of making it as abstract and returning with empty string in ReduceTaskImpl.java. Agree, it will be similar that way. I have updated the patch accordingly. Thanks
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12486224/MAPREDUCE-2661_rev3.patch
        against trunk revision 1145679.

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/459//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/12486224/MAPREDUCE-2661_rev3.patch against trunk revision 1145679. +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/459//console This message is automatically generated.
        Hide
        Ahmed Radwan added a comment -

        Typo:
        I meant 'simpler' in my previous comment: "it will be simpler that way", not 'similar'.

        Show
        Ahmed Radwan added a comment - Typo: I meant 'simpler' in my previous comment: "it will be simpler that way", not 'similar'.
        Hide
        Ahmed Radwan added a comment -

        Did anyone had a chance to review the updated patch? Thanks!

        Show
        Ahmed Radwan added a comment - Did anyone had a chance to review the updated patch? Thanks!
        Hide
        Sharad Agarwal added a comment -

        I committed to MR-279 branch. Thanks Ahmed!

        Show
        Sharad Agarwal added a comment - I committed to MR-279 branch. Thanks Ahmed!

          People

          • Assignee:
            Ahmed Radwan
            Reporter:
            Ahmed Radwan
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development