Hadoop Common
  1. Hadoop Common
  2. HADOOP-8350

Improve NetUtils.getInputStream to return a stream which has a tunable timeout

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0, 2.0.0-alpha
    • Fix Version/s: 2.0.0-alpha
    • Component/s: util
    • Labels:
      None

      Description

      Currently, NetUtils.getInputStream will set the timeout on the new stream based on the socket's configured timeout at the time of construction. After that, the timeout cannot be changed. This causes a problem for cases like HDFS-3357. One approach used in some places in the code is to construct new streams when the timeout has to be changed, but this can cause bugs given that the streams are often wrapped by BufferedInputStreams.

      1. hadoop-8350.txt
        22 kB
        Todd Lipcon
      2. hadoop-8350.txt
        22 kB
        Todd Lipcon
      3. hadoop-8350.txt
        21 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          This isn't a public API:

          @InterfaceAudience.LimitedPrivate({"HDFS", "MapReduce"})
          @InterfaceStability.Unstable
          public class NetUtils {
            private static final Log LOG = LogFactory.getLog(NetUtils.class);
          

          It looks like you're using a binary built against a different version than you're running against.

          Show
          Todd Lipcon added a comment - This isn't a public API: @InterfaceAudience.LimitedPrivate({ "HDFS" , "MapReduce" }) @InterfaceStability.Unstable public class NetUtils { private static final Log LOG = LogFactory.getLog(NetUtils.class); It looks like you're using a binary built against a different version than you're running against.
          Hide
          Ted Yu added a comment -

          I used reflection in HBase to make code work for both hadoop 1.0 and 2.0

          Show
          Ted Yu added a comment - I used reflection in HBase to make code work for both hadoop 1.0 and 2.0
          Hide
          Ted Yu added a comment -

          Here were the commands I used last night:

            368  mvn help:active-profiles -Dhadoop.profile=2.0 -DskipTests install
            369  nohup mvn help:active-profiles -Dhadoop.profile=2.0 test > ../suite.txt &
          
          Show
          Ted Yu added a comment - Here were the commands I used last night: 368 mvn help:active-profiles -Dhadoop.profile=2.0 -DskipTests install 369 nohup mvn help:active-profiles -Dhadoop.profile=2.0 test > ../suite.txt &
          Hide
          Ted Yu added a comment -

          Searching through hadoop 2.0 code base, I think only DataXceiver.java uses the setTimeout() feature of SocketInputWrapper.

          Since SocketInputWrapper is marked unstable, we should try to reduce the impact of API change to downstream projects.
          e.g. getInputStream() should still return InputStream rather than returning SocketInputWrapper.

          DataXceiver can cast this return value to SocketInputWrapper and call setTimeout() on the cast.

          Show
          Ted Yu added a comment - Searching through hadoop 2.0 code base, I think only DataXceiver.java uses the setTimeout() feature of SocketInputWrapper. Since SocketInputWrapper is marked unstable, we should try to reduce the impact of API change to downstream projects. e.g. getInputStream() should still return InputStream rather than returning SocketInputWrapper. DataXceiver can cast this return value to SocketInputWrapper and call setTimeout() on the cast.
          Hide
          Ted Yu added a comment -

          I was running HBase test suite against hadoop 2.0 and observed the following test failure:

          testCalls(org.apache.hadoop.hbase.ipc.TestPBOnWritableRpc)  Time elapsed: 0.002 sec  <<< ERROR!
          java.io.IOException: java.lang.NoSuchMethodError: org.apache.hadoop.net.NetUtils.getInputStream(Ljava/net/Socket;)Ljava/io/InputStream;
          ...
          Caused by: java.lang.NoSuchMethodError: org.apache.hadoop.net.NetUtils.getInputStream(Ljava/net/Socket;)Ljava/io/InputStream;
                  at org.apache.hadoop.hbase.ipc.HBaseClient$Connection.setupIOstreams(HBaseClient.java:693)
                  at org.apache.hadoop.hbase.ipc.HBaseClient.getConnection(HBaseClient.java:1305)
                  at org.apache.hadoop.hbase.ipc.HBaseClient.call(HBaseClient.java:1157)
                  at org.apache.hadoop.hbase.ipc.WritableRpcEngine$Invoker.invoke(WritableRpcEngine.java:152)
                  at $Proxy37.getProtocolVersion(Unknown Source)
          

          Here was the command I used:

          mvn help:active-profiles -Dhadoop.profile=2.0 test
          

          I believe the above exception was caused by this JIRA.

          Show
          Ted Yu added a comment - I was running HBase test suite against hadoop 2.0 and observed the following test failure: testCalls(org.apache.hadoop.hbase.ipc.TestPBOnWritableRpc) Time elapsed: 0.002 sec <<< ERROR! java.io.IOException: java.lang.NoSuchMethodError: org.apache.hadoop.net.NetUtils.getInputStream(Ljava/net/Socket;)Ljava/io/InputStream; ... Caused by: java.lang.NoSuchMethodError: org.apache.hadoop.net.NetUtils.getInputStream(Ljava/net/Socket;)Ljava/io/InputStream; at org.apache.hadoop.hbase.ipc.HBaseClient$Connection.setupIOstreams(HBaseClient.java:693) at org.apache.hadoop.hbase.ipc.HBaseClient.getConnection(HBaseClient.java:1305) at org.apache.hadoop.hbase.ipc.HBaseClient.call(HBaseClient.java:1157) at org.apache.hadoop.hbase.ipc.WritableRpcEngine$Invoker.invoke(WritableRpcEngine.java:152) at $Proxy37.getProtocolVersion(Unknown Source) Here was the command I used: mvn help:active-profiles -Dhadoop.profile=2.0 test I believe the above exception was caused by this JIRA.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1069 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1069/)
          Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650)
          HADOOP-8350. Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1069 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1069/ ) Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650) HADOOP-8350 . Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1034 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1034/)
          Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650)
          HADOOP-8350. Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649)

          Result = FAILURE
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1034 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1034/ ) Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650) HADOOP-8350 . Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649) Result = FAILURE todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2201 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2201/)
          Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650)
          HADOOP-8350. Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649)

          Result = ABORTED
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2201 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2201/ ) Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650) HADOOP-8350 . Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649) Result = ABORTED todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2257 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2257/)
          Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650)
          HADOOP-8350. Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2257 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2257/ ) Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650) HADOOP-8350 . Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2183 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2183/)
          Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650)
          HADOOP-8350. Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2183 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2183/ ) Amend previous commit of HADOOP-8350 (missed new SocketInputWrapper file) (Revision 1333650) HADOOP-8350 . Improve NetUtils.getInputStream to return a stream which has a tunable timeout. Contributed by Todd Lipcon. (Revision 1333649) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333650 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputWrapper.java todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1333649 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketIOWithTimeout.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/SocketInputStream.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestNetUtils.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestSocketIOWithTimeout.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
          Hide
          Todd Lipcon added a comment -

          Committed to branch-2 and trunk. Thanks for reviews, Tom and Eli.

          Show
          Todd Lipcon added a comment - Committed to branch-2 and trunk. Thanks for reviews, Tom and Eli.
          Hide
          Todd Lipcon added a comment -

          New patch uses constant in TestSocketIOWithTimeout as well. I'll commit this version per Eli's +1 above.

          Show
          Todd Lipcon added a comment - New patch uses constant in TestSocketIOWithTimeout as well. I'll commit this version per Eli's +1 above.
          Hide
          Eli Collins added a comment -

          +1 (Nit: TestSocketIOWithTimeout should use TIME_FUDGE_MILLIS instead of 200, feel free to address w/o a new patch)

          Show
          Eli Collins added a comment - +1 (Nit: TestSocketIOWithTimeout should use TIME_FUDGE_MILLIS instead of 200, feel free to address w/o a new patch)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525510/hadoop-8350.txt
          against trunk revision .

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.fs.viewfs.TestViewFsTrash

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/932//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/932//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/12525510/hadoop-8350.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. -1 javadoc. The javadoc tool appears to have generated 2 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.fs.viewfs.TestViewFsTrash +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/932//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/932//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          New patch addresses feedback above.

          Show
          Todd Lipcon added a comment - New patch addresses feedback above.
          Hide
          Eli Collins added a comment -

          I noticed that 200 is already used as a magic # in TestSocketIOWithTimeout as well (so should be well-tested), perhaps worth a define used in both places.

          Show
          Eli Collins added a comment - I noticed that 200 is already used as a magic # in TestSocketIOWithTimeout as well (so should be well-tested), perhaps worth a define used in both places.
          Hide
          Tom White added a comment -

          Overall looks good, +1.

          A couple of nits: in TestNetUtils, there is a magic number of 200 in assertTimeSince, and on assertReadTimeout maybe call the second parameter expectedMillis (since its not clear if it's nanos or millis).

          Show
          Tom White added a comment - Overall looks good, +1. A couple of nits: in TestNetUtils, there is a magic number of 200 in assertTimeSince, and on assertReadTimeout maybe call the second parameter expectedMillis (since its not clear if it's nanos or millis).
          Hide
          Eli Collins added a comment - - edited

          Agree we need this, that eg in HDFS-3357 the right approach is not to re-create the streams to set a timeout. It's definitely worth adding a note wrt having a single SocketInputWrapper per socket to NetUtils#getInputStream so callers are aware of the behavior described in SocketInputWrapper#setTimeout. Otherwise +1, looks excellent.

          Show
          Eli Collins added a comment - - edited Agree we need this, that eg in HDFS-3357 the right approach is not to re-create the streams to set a timeout. It's definitely worth adding a note wrt having a single SocketInputWrapper per socket to NetUtils#getInputStream so callers are aware of the behavior described in SocketInputWrapper#setTimeout. Otherwise +1, looks excellent.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525490/hadoop-8350.txt
          against trunk revision .

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

          +1 tests included. The patch appears to include 2 new or modified test files.

          -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.fs.viewfs.TestViewFsTrash

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/929//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/929//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/12525490/hadoop-8350.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified test files. -1 javadoc. The javadoc tool appears to have generated 2 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.fs.viewfs.TestViewFsTrash +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/929//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/929//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Attached patch does the following:

          • introduces a new SocketInputWrapper class. In the case that the socket has a channel, this wraps the existing NIO-based SocketInputStream implementation. Otherwise, it just wraps the InputStream provided by Java's Socket.getInputStream. This wrapper also contains a call to set the timeout, which either propagates the timeout to the NIO implementation's SocketIOWithTimeout, or propagates to Socket.setTimeout(), as appropriate.
          • sets SocketInputStream back to being package-private, now that its functionality is exposed by SocketInputWrapper. It used to be this way, and it's always been annotated LimitedPrivate, so this isn't incompatible.
          • Fixed TestSocketIOWithTimeout to use MultithreadedTestUtil.TestContext. It previously assumed that "fail()" in an inferior thread would cause the test case to fail, which is not the case. Also amended this test to test the setTimeout() call.

          There's one small cross-project change to HDFS's RemoteBlockReader2 here to use the new API. I ran a few HDFS client test cases manually to make sure I didn't break anything there.

          Show
          Todd Lipcon added a comment - Attached patch does the following: introduces a new SocketInputWrapper class. In the case that the socket has a channel, this wraps the existing NIO-based SocketInputStream implementation. Otherwise, it just wraps the InputStream provided by Java's Socket.getInputStream. This wrapper also contains a call to set the timeout, which either propagates the timeout to the NIO implementation's SocketIOWithTimeout, or propagates to Socket.setTimeout(), as appropriate. sets SocketInputStream back to being package-private, now that its functionality is exposed by SocketInputWrapper. It used to be this way, and it's always been annotated LimitedPrivate, so this isn't incompatible. Fixed TestSocketIOWithTimeout to use MultithreadedTestUtil.TestContext. It previously assumed that "fail()" in an inferior thread would cause the test case to fail, which is not the case. Also amended this test to test the setTimeout() call. There's one small cross-project change to HDFS's RemoteBlockReader2 here to use the new API. I ran a few HDFS client test cases manually to make sure I didn't break anything there.

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              1 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development