Juan Yu has posted comments on this change. Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout ......................................................................
Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/3343/9//COMMIT_MSG Commit Message: PS9, Line 9: configable > configurable Done PS9, Line 21: keep default timeout to 0 > maybe mention in brackets that this means the timeouts are disabled. Done Line 24: > Also mention the WaitRpcResp() Done PS9, Line 25: we don't put it back to cache > "we don't put the underlying connection back in the cache" Done 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) Done 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 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 Done 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:" Done -- 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 <j...@cloudera.com> Gerrit-Reviewer: Alan Choi <a...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
