Juan Yu has posted comments on this change. Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout ......................................................................
Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/3343/5/be/src/runtime/backend-client.h File be/src/runtime/backend-client.h: PS5, Line 48: // Consider back pressure, we should wait for receiver side as long as possible : // but not forever. Also we need to make sure Cancel() can stop the waiting. > I preferred the old way where you could set per-RPC timeouts. You can combi I have the same concern as Sailesh, there might be performance impact if we set timeout per RPC. Also maintain a different timeout for each type of RPC could be hard. I agree move retry to client is better. http://gerrit.cloudera.org:8080/#/c/3343/5/be/src/runtime/client-cache.h File be/src/runtime/client-cache.h: PS5, Line 240: Reopen() This could causing lots of issues. Without checking exception details, we don't know if the exception is from socket send() or recv(), simply retry both rpc could lead to send the request multiple times. For example, ExecutePlanFragment() called twice for the same fragment, the same row batch is sent twice, etc. http://gerrit.cloudera.org:8080/#/c/3343/5/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS5, Line 1490: // Try to send the RPC 3 times before failing. : for (int i = 0; i < 3; ++i) { > This always sends the RPC three times if it succeeds every time. You're right, I should check if status is ok. PS5, Line 1494: if (rpc_status.code() == TErrorCode::RPC_TIMEOUT) break; > If you abort after a timeout error, what's the rationale for sending the RP IsTimeoutTException() checks for a specific timedout that can only happen at socket recv(), which means send() is already done. remote node gets the cancel request, no need to retry. -- 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: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Juan Yu <[email protected]> Gerrit-Reviewer: Alan Choi <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
