Juan Yu has posted comments on this change. Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout ......................................................................
Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS10, Line 1495: rpc_status = backend_client.WaitRpcResp( : &ImpalaBackendClient::recv_CancelPlanFragment, &res, runtime_state(), 100); > By this stage, the send() part of the RPC has already succeeded right? So, yes, you're right. I like the idea of adding a retry flag. http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/data-stream-sender.cc File be/src/runtime/data-stream-sender.cc: PS10, Line 230: Status DataStreamSender::Channel::DoRpcTimedWait(ImpalaBackendConnection* client, : const TTransmitDataParams& params, TTransmitDataResult* res, : uint64_t timeout) { : 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); : } : return status; : } > This would be more cleaner if done as a function in client-cache rather tha Done http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: PS10, Line 560: if (IsCompleted()) { > What if 'done_' is true here? when done_ is set, PlanFragmentExecutor::FragmentComplete() is always called and status is reported back to coordinator and report thread will be stopped. check done_ here could accidentally stop report thread too soon and cause the fragment completed status not send to coordinator. http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/plan-fragment-executor.h File be/src/runtime/plan-fragment-executor.h: PS10, Line 146: IsCompleted() > This technically should not be IsCompleted() because we check if (!done_). since it's only used once, I'll just check the vars directly instead of using a function. -- 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: 10 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
