Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: task
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      While looking at the code with Amareshwari, we realized that Fetcher#copyFromHost marks connection as successful when url.openConnection returns. This is wrong. Connection is done inside implicitly inside getInputStream; we need to split getInputStream into connect and getInputStream to handle the connection and read time out strategies correctly.

      1. ASF.LICENSE.NOT.GRANTED--patch-1276.txt
        4 kB
        Amareshwari Sriramadasu
      2. patch-1276-1.txt
        4 kB
        Amareshwari Sriramadasu
      3. patch-1276-2.txt
        4 kB
        Amareshwari Sriramadasu

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #323 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/323/)
        MAPREDUCE-1276. Correct flaws in the shuffle related to connection setup and
        failure attribution. Contributed by Amareshwari Sriramadasu

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #323 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/323/ ) MAPREDUCE-1276 . Correct flaws in the shuffle related to connection setup and failure attribution. Contributed by Amareshwari Sriramadasu
        Hide
        Chris Douglas added a comment -

        +1

        I committed this. Thanks, Amareshwari!

        Show
        Chris Douglas added a comment - +1 I committed this. Thanks, Amareshwari!
        Hide
        Amareshwari Sriramadasu added a comment -

        -1 core tests.

        Test failure TestSetupAndCleanupFailure is not related to the patch. It is because of ZipException.
        The same test passed on my machine.

        Show
        Amareshwari Sriramadasu added a comment - -1 core tests. Test failure TestSetupAndCleanupFailure is not related to the patch. It is because of ZipException. The same test passed on my machine.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12443815/patch-1276-2.txt
        against trunk revision 941564.

        +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 failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/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/12443815/patch-1276-2.txt against trunk revision 941564. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/364/console This message is automatically generated.
        Hide
        Amareshwari Sriramadasu added a comment -

        Repeated the manual test with patch, it works as expected now.

        Show
        Amareshwari Sriramadasu added a comment - Repeated the manual test with patch, it works as expected now.
        Hide
        Amareshwari Sriramadasu added a comment -

        Patch marks exception during outStream.flush() as not an input exception. Also, moves mapOutputIn.close() to finally block.

        Show
        Amareshwari Sriramadasu added a comment - Patch marks exception during outStream.flush() as not an input exception. Also, moves mapOutputIn.close() to finally block.
        Hide
        Chris Douglas added a comment -

        I agree; failing to flush the output buffer clearly shouldn't count against the map output. Closing mapOutputIn should probably be in a finally block.

        Show
        Chris Douglas added a comment - I agree; failing to flush the output buffer clearly shouldn't count against the map output. Closing mapOutputIn should probably be in a finally block.
        Hide
        Amareshwari Sriramadasu added a comment -

        I repeated the manual testing described in my earlier comment.
        I tried to simulate read timeout for m_00001_0 by explicitly adding a sleep in TaskTracker.MapOutputServlet.sendMapFile(). The attempt fails with error "Too many fetch failures" as expected.
        But most of the times I see m_00002_0 also failing with following error:

        Map output lost, rescheduling: error on sending map attempt_201005051443_0003_m_000002_0 to reduce 1
        org.mortbay.jetty.EofException 
        at org.mortbay.jetty.HttpGenerator.flush(HttpGenerator.java:787) 
        at org.mortbay.jetty.AbstractGenerator$Output.flush(AbstractGenerator.java:566) 
        at org.mortbay.jetty.HttpConnection$Output.flush(HttpConnection.java:946) 
        at java.io.DataOutputStream.flush(DataOutputStream.java:106) 
        at org.apache.hadoop.mapred.TaskTracker$MapOutputServlet.sendMapFile(TaskTracker.java:3646) 
        at org.apache.hadoop.mapred.TaskTracker$MapOutputServlet.doGet(TaskTracker.java:3517) 
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:707) 
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:820) 
        at org.mortbay.jetty.servlet.ServletHolder.handle(ServletHolder.java:502) 
        at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1124) 
        at org.apache.hadoop.http.HttpServer$QuotingInputFilter.doFilter(HttpServer.java:766) at
        ..
        

        Jothi/Chris, do you think this is an agreeable failure?
        I think we should catch this as not an inputException and do a retry.

        Show
        Amareshwari Sriramadasu added a comment - I repeated the manual testing described in my earlier comment. I tried to simulate read timeout for m_00001_0 by explicitly adding a sleep in TaskTracker.MapOutputServlet.sendMapFile(). The attempt fails with error "Too many fetch failures" as expected. But most of the times I see m_00002_0 also failing with following error: Map output lost, rescheduling: error on sending map attempt_201005051443_0003_m_000002_0 to reduce 1 org.mortbay.jetty.EofException at org.mortbay.jetty.HttpGenerator.flush(HttpGenerator.java:787) at org.mortbay.jetty.AbstractGenerator$Output.flush(AbstractGenerator.java:566) at org.mortbay.jetty.HttpConnection$Output.flush(HttpConnection.java:946) at java.io.DataOutputStream.flush(DataOutputStream.java:106) at org.apache.hadoop.mapred.TaskTracker$MapOutputServlet.sendMapFile(TaskTracker.java:3646) at org.apache.hadoop.mapred.TaskTracker$MapOutputServlet.doGet(TaskTracker.java:3517) at javax.servlet.http.HttpServlet.service(HttpServlet.java:707) at javax.servlet.http.HttpServlet.service(HttpServlet.java:820) at org.mortbay.jetty.servlet.ServletHolder.handle(ServletHolder.java:502) at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1124) at org.apache.hadoop.http.HttpServer$QuotingInputFilter.doFilter(HttpServer.java:766) at .. Jothi/Chris, do you think this is an agreeable failure? I think we should catch this as not an inputException and do a retry.
        Hide
        Amareshwari Sriramadasu added a comment -

        Attached patch removes the drain buffer code and replaces input.close() with IOUtils.cleanup().

        Show
        Amareshwari Sriramadasu added a comment - Attached patch removes the drain buffer code and replaces input.close() with IOUtils.cleanup().
        Hide
        Jothi Padmanabhan added a comment -

        While I added the "drain" code, we were debugging some issues with length corruption, which have since been attributed to some bad jdk version (u13 IIRC). We have not seen any corruptions since. So, I do not think the drain code is very critical, it is added to handle some really rare situations. In the same vein, the drain code itself will be executed on such rare cases as well. If input.close does the necessary job, I am fine with removing this code. And yes +1 for using IOUtils::cleanup.

        Show
        Jothi Padmanabhan added a comment - While I added the "drain" code, we were debugging some issues with length corruption, which have since been attributed to some bad jdk version (u13 IIRC). We have not seen any corruptions since. So, I do not think the drain code is very critical, it is added to handle some really rare situations. In the same vein, the drain code itself will be executed on such rare cases as well. If input.close does the necessary job, I am fine with removing this code. And yes +1 for using IOUtils::cleanup.
        Hide
        Chris Douglas added a comment -

        Oh, OK; I didn't know that. While this prevents the connection from being reused, it will still be garbage collected if it is simply closed, right? The current code prevents another Fetcher from fetching output from that host while the socket is "drained," which seems like an avoidable cost. Leaving the TCP connection open is regrettable... thoughts on this tradeoff?

        • If retained, would it be possible to update the comment on the drain code? The current explanation, "just in case", makes it sound like superstition instead of a deliberate choice
        • Since input.close() can also throw, would it make sense to do that within the try/catch or use IOUtils::cleanup?
        Show
        Chris Douglas added a comment - Oh, OK; I didn't know that. While this prevents the connection from being reused, it will still be garbage collected if it is simply closed, right? The current code prevents another Fetcher from fetching output from that host while the socket is "drained," which seems like an avoidable cost. Leaving the TCP connection open is regrettable... thoughts on this tradeoff? If retained, would it be possible to update the comment on the drain code? The current explanation, "just in case", makes it sound like superstition instead of a deliberate choice Since input.close() can also throw, would it make sense to do that within the try/catch or use IOUtils::cleanup ?
        Hide
        Jothi Padmanabhan added a comment -

        Why is the input stream drained at all

        As per http://java.sun.com/j2se/1.5.0/docs/guide/net/http-keepalive.html, it is a good practice to read the entire response body if getInputStream returns successfully. So, if getInputStream returns success but there is some corruption in data (say some length mismatch), it might be a good idea to siphon off the data from the socket.

        Show
        Jothi Padmanabhan added a comment - Why is the input stream drained at all As per http://java.sun.com/j2se/1.5.0/docs/guide/net/http-keepalive.html , it is a good practice to read the entire response body if getInputStream returns successfully. So, if getInputStream returns success but there is some corruption in data (say some length mismatch), it might be a good idea to siphon off the data from the socket.
        Hide
        Chris Douglas added a comment -

        I apologize for my delay. I will commit this to the 0.21 branch, also.

        +1 on most of these changes. Great catches!

        If there is connect or read timeout during draining the input stream of the connection, the reducer fails.
        Solution : If there is any timeout here, we should ignore and let the reducer run.

        Why is the input stream drained at all? Catching the exception is the right call, but protecting close should be sufficient, right? Removing the drain code and using IOUtils::cleanup instead would make sense.

        Show
        Chris Douglas added a comment - I apologize for my delay. I will commit this to the 0.21 branch, also. +1 on most of these changes. Great catches! If there is connect or read timeout during draining the input stream of the connection, the reducer fails. Solution : If there is any timeout here, we should ignore and let the reducer run. Why is the input stream drained at all? Catching the exception is the right call, but protecting close should be sufficient, right? Removing the drain code and using IOUtils::cleanup instead would make sense.
        Hide
        Amareshwari Sriramadasu added a comment -

        Chris, can you please have a look at the patch? This is a real blocker for 0.21 release.

        Show
        Amareshwari Sriramadasu added a comment - Chris, can you please have a look at the patch? This is a real blocker for 0.21 release.
        Hide
        Amareshwari Sriramadasu added a comment -

        Test output from hudson's console :
        [exec]
        [exec] -1 overall. Here are the results of testing the latest attachment
        [exec] http://issues.apache.org/jira/secure/attachment/12441703/patch-1276.txt
        [exec] against trunk revision 933441.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
        [exec] Please justify why no new tests are needed for this patch.
        [exec] Also please list what manual steps were performed to verify this patch.
        [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 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec] +1 core tests. The patch passed core unit tests.
        [exec]
        [exec] -1 contrib tests. The patch failed contrib unit tests.
        [exec]
        [exec] Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/testReport/
        [exec] Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        [exec] Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/artifact/trunk/build/test/checkstyle-errors.html
        [exec] Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/console

        -1 contrib tests

        Contrib test failures are because NoClassDefFoundError (MAPREDUCE-1275).

        -1 tests included.

        It is difficult to write a Junit test for simulating Read or Connect Timeout in shuffle. I created MAPREDUCE-1708 to add a fault injection test.
        I manually tested the patch by adding sleep in TaskTracker.MapOutputServlet.sendMapFile for one of the attempts. Verified the attempt gets failed because of "Too many fetch failures"; re-executes on some other machine; and the job succeeds.

        Show
        Amareshwari Sriramadasu added a comment - Test output from hudson's console : [exec] [exec] -1 overall. Here are the results of testing the latest attachment [exec] http://issues.apache.org/jira/secure/attachment/12441703/patch-1276.txt [exec] against trunk revision 933441. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no new tests are needed for this patch. [exec] Also please list what manual steps were performed to verify this patch. [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 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] +1 core tests. The patch passed core unit tests. [exec] [exec] -1 contrib tests. The patch failed contrib unit tests. [exec] [exec] Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/testReport/ [exec] Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html [exec] Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/artifact/trunk/build/test/checkstyle-errors.html [exec] Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/108/console -1 contrib tests Contrib test failures are because NoClassDefFoundError ( MAPREDUCE-1275 ). -1 tests included. It is difficult to write a Junit test for simulating Read or Connect Timeout in shuffle. I created MAPREDUCE-1708 to add a fault injection test. I manually tested the patch by adding sleep in TaskTracker.MapOutputServlet.sendMapFile for one of the attempts. Verified the attempt gets failed because of "Too many fetch failures"; re-executes on some other machine; and the job succeeds.
        Hide
        Amareshwari Sriramadasu added a comment -

        When I tried to simulate a read timeout for one of the attempts on a TaskTracker, I hit few bugs with new shuffle code.
        1. Connection logic is wrong as the jira description says. So, both connect timeout and read timeout are treated as read timeout now.
        Solution: Separate connect() and getInputStream() into different blocks as in pre-0.21.

        2. If there is connect or read timeout during draining the input stream of the connection, the reducer fails.
        Solution : If there is any timeout here, we should ignore and let the reducer run.

        3. If a tracker ran map tasks 1, 2, 3 and tracker times out reading the output of 2nd task, the reducer penalizes the 1st task. This is because the reducer tries to read all the three map outputs in one connection.
        Solution: After reading every map output, we should do a flush() so that the correct map will be penalized for any error.

        4. If a successful task is marked as FAILED because of "Too many fetch failures", the task is not removed from reduce.Fetcher's known pending maps. So, reducer will indefinitely try to read the FAILED map.
        Solution: EventFetcher does not add FAILED/KILLED TaskCompletionEvents to obsolete maps. We should add them also as we did in pre-0.21.

        Attached patch fixes all the above bugs.

        Show
        Amareshwari Sriramadasu added a comment - When I tried to simulate a read timeout for one of the attempts on a TaskTracker, I hit few bugs with new shuffle code. 1. Connection logic is wrong as the jira description says. So, both connect timeout and read timeout are treated as read timeout now. Solution: Separate connect() and getInputStream() into different blocks as in pre-0.21. 2. If there is connect or read timeout during draining the input stream of the connection, the reducer fails. Solution : If there is any timeout here, we should ignore and let the reducer run. 3. If a tracker ran map tasks 1, 2, 3 and tracker times out reading the output of 2nd task, the reducer penalizes the 1st task. This is because the reducer tries to read all the three map outputs in one connection. Solution: After reading every map output, we should do a flush() so that the correct map will be penalized for any error. 4. If a successful task is marked as FAILED because of "Too many fetch failures", the task is not removed from reduce.Fetcher's known pending maps. So, reducer will indefinitely try to read the FAILED map. Solution: EventFetcher does not add FAILED/KILLED TaskCompletionEvents to obsolete maps. We should add them also as we did in pre-0.21. Attached patch fixes all the above bugs.

          People

          • Assignee:
            Amareshwari Sriramadasu
            Reporter:
            Jothi Padmanabhan
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development