Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    1. patch-5698.txt
      118 kB
      Amareshwari Sriramadasu
    2. patch-5698-1.txt
      122 kB
      Amareshwari Sriramadasu

      Issue Links

        Activity

        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #867 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/867/ )
        Hide
        Sharad Agarwal added a comment -

        I just committed this. Thanks Amareshwari!

        Show
        Sharad Agarwal added a comment - I just committed this. Thanks Amareshwari!
        Hide
        Amareshwari Sriramadasu added a comment -

        ant test passed on my machine

        Show
        Amareshwari Sriramadasu added a comment - ant test passed on my machine
        Hide
        dhruba borthakur added a comment -

        +1 Code looks great!

        Show
        dhruba borthakur added a comment - +1 Code looks great!
        Hide
        Amareshwari Sriramadasu added a comment -

        I think it would be a better idea not to club the fixes to the existing mapred.CombineInputFormat into this Jira. That should be addressed in a separate Jira and the patch for this should be built on top of that.

        Done by HADOOP-5759

        # Minor - Why is there a return 2 in run (instead of return 1 as in existing code)

        exit code is 2 if the usage was wrong

        CombineFileInputFormat.createRecordReader - should this just return null or should it call super.createRecordReader

        Implemented abstract method from super class to return null. commented the same

        test-patch :

             [exec] +1 overall.
             [exec]
             [exec]     +1 @author.  The patch does not contain any @author tags.
             [exec]
             [exec]     +1 tests included.  The patch appears to include 9 new or modified tests.
             [exec]
             [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
             [exec]
             [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
             [exec]
             [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
             [exec]
             [exec]     +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
             [exec]
             [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
             [exec]
        
        Show
        Amareshwari Sriramadasu added a comment - I think it would be a better idea not to club the fixes to the existing mapred.CombineInputFormat into this Jira. That should be addressed in a separate Jira and the patch for this should be built on top of that. Done by HADOOP-5759 # Minor - Why is there a return 2 in run (instead of return 1 as in existing code) exit code is 2 if the usage was wrong CombineFileInputFormat.createRecordReader - should this just return null or should it call super.createRecordReader Implemented abstract method from super class to return null. commented the same test-patch : [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 9 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec]
        Hide
        Amareshwari Sriramadasu added a comment -

        Patch with review comments incorporated.

        Show
        Amareshwari Sriramadasu added a comment - Patch with review comments incorporated.
        Hide
        Hadoop QA added a comment -

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

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

        +1 tests included. The patch appears to include 9 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 javac. The applied patch generated 801 javac compiler warnings (more than the trunk's current 2455 warnings).

        -1 findbugs. The patch appears to cause Findbugs to fail.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

        +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/Hadoop-Patch-vesta.apache.org/269/testReport/
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/269/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/269/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/12406761/patch-5698.txt against trunk revision 770685. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 801 javac compiler warnings (more than the trunk's current 2455 warnings). -1 findbugs. The patch appears to cause Findbugs to fail. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +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/Hadoop-Patch-vesta.apache.org/269/testReport/ Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/269/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/269/console This message is automatically generated.
        Hide
        Jothi Padmanabhan added a comment -

        I think it would be a better idea not to club the fixes to the existing mapred.CombineInputFormat into this Jira. That should be addressed in a separate Jira and the patch for this should be built on top of that.

        Some other points:

        1. MultiFileWordCount – I do not think we should use the MultiFileLineRecordReader to read from a CombineSplit. It is guaranteed to work only if the start offset is 0, which is not necessarily true. Instead the CombineFileRecordReader should be used
        2. Minor – Why is there a return 2 in run (instead of return 1 as in existing code)
        3. CombineFileInputFormat.createRecordReader – should this just return null or should it call super.createRecordReader ?
        4. Minor – CombineFileRecordReader – Remove unused exports
        5. Minor – Where ever possible, keep the code/comments restricted to 80 columns
        Show
        Jothi Padmanabhan added a comment - I think it would be a better idea not to club the fixes to the existing mapred.CombineInputFormat into this Jira. That should be addressed in a separate Jira and the patch for this should be built on top of that. Some other points: MultiFileWordCount – I do not think we should use the MultiFileLineRecordReader to read from a CombineSplit. It is guaranteed to work only if the start offset is 0, which is not necessarily true. Instead the CombineFileRecordReader should be used Minor – Why is there a return 2 in run (instead of return 1 as in existing code) CombineFileInputFormat.createRecordReader – should this just return null or should it call super.createRecordReader ? Minor – CombineFileRecordReader – Remove unused exports Minor – Where ever possible, keep the code/comments restricted to 80 columns
        Hide
        Amareshwari Sriramadasu added a comment -

        Patch moves CombineFileInputFormat, CombineFileRecordReader, CombineFileSplit libraries and org.apache.hadoop.examples.MultiFileWordCount to use new mapreduce api.
        In existing CombineFileInputFormat, the split location passed is rack-name. But this is resulting in IlleagalArgumentException while resolving, since it contains PATH_SEPARATOR('/') in the name. Also passing rackname (without '/'), results in wrong resolution of the node as /default-rack/<rack-name>. After offline discussion with Dhruba, I changed the input format to pass hostnames instead of racknames as a solution to this problem.

        Show
        Amareshwari Sriramadasu added a comment - Patch moves CombineFileInputFormat, CombineFileRecordReader, CombineFileSplit libraries and org.apache.hadoop.examples.MultiFileWordCount to use new mapreduce api. In existing CombineFileInputFormat, the split location passed is rack-name. But this is resulting in IlleagalArgumentException while resolving, since it contains PATH_SEPARATOR('/') in the name. Also passing rackname (without '/'), results in wrong resolution of the node as /default-rack/<rack-name>. After offline discussion with Dhruba, I changed the input format to pass hostnames instead of racknames as a solution to this problem.

          People

          • Assignee:
            Amareshwari Sriramadasu
            Reporter:
            Amareshwari Sriramadasu
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development