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

PipeMapRed.java has uninitialized members log_ and LOGNAME

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: contrib/streaming
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      PipeMapRed.java has members log_ and LOGNAME, which are never initialized and they are used in code for logging in several places.
      They should be removed and PipeMapRed should use commons LogFactory and Log for logging. This would improve code maintainability.

      Also, as per comment , stream.joblog_ configuration property can be removed.

      1. patch-1864.txt
        12 kB
        Amareshwari Sriramadasu

        Activity

        Hide
        Ravi Gummadi added a comment -

        One more unused member is debugFailEarly_

        Show
        Ravi Gummadi added a comment - One more unused member is debugFailEarly_
        Hide
        Ravi Gummadi added a comment -

        Also, I don't see a real need for the following as part of code.

        boolean debug_;
        boolean debugFailEarly_;
        boolean debugFailDuring_;
        boolean debugFailLate_;

        If some debug statements are needed, we could add them as

         if (LOG.isDebugEnabled()) {LOG.debug(......);} 

        So should we remove these 4 variables also ?

        Show
        Ravi Gummadi added a comment - Also, I don't see a real need for the following as part of code. boolean debug_; boolean debugFailEarly_; boolean debugFailDuring_; boolean debugFailLate_; If some debug statements are needed, we could add them as if (LOG.isDebugEnabled()) {LOG.debug(......);} So should we remove these 4 variables also ?
        Hide
        Ravi Gummadi added a comment -

        mapredKey_ is not set at all but referenced in getContext(). We need to set it appropriately.

        Show
        Ravi Gummadi added a comment - mapredKey_ is not set at all but referenced in getContext(). We need to set it appropriately.
        Hide
        Amareshwari Sriramadasu added a comment -

        Patch removes un-initialized and unused variables : fs_, debug_, debugFailEarly_, debugFailDuring_, debugFailLate_, jobLog_, log_, LOGNAME and mapredKey_.

        If some debug statements are needed, we could add them as..

        Two of the debug statements do not look relevant, so removed them. Replaced the other one with LOG.debug.

        mapredKey_ is not set at all but referenced in getContext(). We need to set it appropriately.

        The variable is never set from the first version. The next line in getContext() is adding last output. So, removed this.

        Also removed new Date() from getContext(), because getContext() is called from log message and logger will anyway prefix the log with the current time.

        Show
        Amareshwari Sriramadasu added a comment - Patch removes un-initialized and unused variables : fs_, debug_, debugFailEarly_, debugFailDuring_, debugFailLate_, jobLog_, log_, LOGNAME and mapredKey_. If some debug statements are needed, we could add them as.. Two of the debug statements do not look relevant, so removed them. Replaced the other one with LOG.debug. mapredKey_ is not set at all but referenced in getContext(). We need to set it appropriately. The variable is never set from the first version. The next line in getContext() is adding last output. So, removed this. Also removed new Date() from getContext(), because getContext() is called from log message and logger will anyway prefix the log with the current time.
        Hide
        Hadoop QA added a comment -

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

        +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 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-h6.grid.sp2.yahoo.net/586/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/586/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/586/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/586/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/12447931/patch-1864.txt against trunk revision 957437. +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 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-h6.grid.sp2.yahoo.net/586/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/586/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/586/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/586/console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

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

        +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 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-h4.grid.sp2.yahoo.net/273/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/273/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/273/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/273/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/12447931/patch-1864.txt against trunk revision 958902. +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 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-h4.grid.sp2.yahoo.net/273/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/273/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/273/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/273/console This message is automatically generated.
        Hide
        Amareshwari Sriramadasu added a comment -

        -1 contrib tests.

        Test failure is due to MAPREDUCE-1834.

        -1 tests included.

        The patch removes unused code. So, no new tests are added.

        Show
        Amareshwari Sriramadasu added a comment - -1 contrib tests. Test failure is due to MAPREDUCE-1834 . -1 tests included. The patch removes unused code. So, no new tests are added.
        Hide
        Ravi Gummadi added a comment -

        Patch looks good.
        +1

        Show
        Ravi Gummadi added a comment - Patch looks good. +1
        Hide
        Amareshwari Sriramadasu added a comment -

        I just committed this!

        Show
        Amareshwari Sriramadasu added a comment - I just committed this!

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development