Sailesh Mukil has posted comments on this change. Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout ......................................................................
Patch Set 9: (8 comments) http://gerrit.cloudera.org:8080/#/c/3343/9//COMMIT_MSG Commit Message: PS9, Line 9: configable configurable PS9, Line 21: keep default timeout to 0 maybe mention in brackets that this means the timeouts are disabled. Line 24: Also mention the WaitRpcResp() PS9, Line 25: we don't put it back to cache "we don't put the underlying connection back in the cache" http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/client-cache.cc File be/src/runtime/client-cache.cc: Line 147: shared_ptr<ThriftClientImpl> client_impl; DCHECK(*client_key != NULL) http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/data-stream-sender.cc File be/src/runtime/data-stream-sender.cc: PS9, Line 216: ONE_HOUR_IN_MS How was one hour chosen? I'm worried this might cause lot of queries to hang for an hour and still end up failing. Are we sure that TErrorCode::RPC_TIMEOUT only means that the timeout happened during recv()? PS9, Line 306: Status status = client.DoRpc(&ImpalaBackendClient::TransmitData, params, &res); : if (status.code() == TErrorCode::RPC_TIMEOUT) { : status = client.WaitRpcResp(&ImpalaBackendClient::recv_TransmitData, &res, : runtime_state_, ONE_HOUR_IN_MS); : } : rpc_status_ = status; This code seems to be reused in many places. Maybe you can create a wrapper function like DoRpcTimedWait() and call that here and everywhere else. http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: PS9, Line 456: Note "Note:" -- To view, visit http://gerrit.cloudera.org:8080/3343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964 Gerrit-PatchSet: 9 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Juan Yu <[email protected]> Gerrit-Reviewer: Alan Choi <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
