functioner edited a comment on pull request #2727:
URL: https://github.com/apache/hadoop/pull/2727#issuecomment-788518502
> > According to the comment in that test case, it "should not time out
because effective rpc-timeout is multiple of ping interval: 1600 (= 800 * (1000
/ 800 + 1))", and it doesn't mean that it shouldn't time out.
>
> The SocketTimeoutException is thrown on `Socket#read` based on the timeout
value set by `Socket#setSoTimeout`. It is set to 800(pingInterval) in the test
because (pingInterval < rpcTimeout). The SocketTimeoutException is thrown at
800ms first time and swallowed because 800 < rpcTimeout. SocketTimeoutException
is thrown again at 1600ms then handled because 1600 > rpcTimeout.
Oh, I think the current fix I propose does not consider the case of
`rpcTimeout != 0`. It always assumes that `rpcTimeout == 0`, and then
calculates `rpcTimeout` based on `pingInterval`.
I'm going to propose:
```
private void handleTimeout(SocketTimeoutException e, int waiting)
throws IOException {
int timeout = 0;
if (rpcTimeout == 0) {
// effective rpc timeout is rounded up to multiple of pingInterval
// if pingInterval < rpcTimeout.
timeout = (doPing) ? pingInterval : rpcTimeout;
} else {
timeout = rpcTimeout;
}
if (shouldCloseConnection.get() || !running.get() ||
(0 < timeout && timeout <= waiting)) {
```
@iwasakims Do you agree?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
