Sailesh Mukil 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, if this call to WaitRpcResp() fails with an error other than RPC_TIMEOUT, wouldn't it loop back and call DoRpc() again, which means that now the RPC is sent twice? Also, it would be useful to add a brief comment explaining why WaitRpcResp() is inside its own loop. 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 than over here. So even the code in the coordinator and the fragment-exec-state can call this function. You could add a retry parameter to the function signature as well. 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? 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_). Maybe think of a different name? -- 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 <[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
