Henry Robinson has posted comments on this change. Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout ......................................................................
Patch Set 16: (6 comments) just looked at the DoRpc*() changes - I think they look much better now. http://gerrit.cloudera.org:8080/#/c/3343/16/be/src/runtime/client-cache.h File be/src/runtime/client-cache.h: Line 226: /// Returns RPC_RECV_TIMEOUT if a timeout occurred, RPC_CLIENT_CONNECT_FAILURE if the "while waiting for a response" Line 254: << e.what() << ", type=" << typeid(e).name(); nit: align << with above PS16, Line 274: or certain RPCs, server could take long time to process it and intentionally block : /// the RPC response to add back pressure. In those cases, client needs to retry recv : /// RPC call to wait longer. suggest: "In certain cases, the server may take longer to provide an RPC response than the configured socket timeout. Callers may wish to retry receiving the response. This is safe if and only if DoRpc() returned RPC_RECV_TIMEOUT and set 'can_retry' to false.". PS16, Line 299: /// Indicate the rpc call sent by this connection succeeds or not. If the rpc call fails : /// for any reason, the connection could be left in a bad state and cannot be reused any : /// more. : bool has_rpc_error_; Why not proactively destroy the connection when this condition is hit? http://gerrit.cloudera.org:8080/#/c/3343/16/be/src/runtime/data-stream-sender.cc File be/src/runtime/data-stream-sender.cc: PS16, Line 228: DoRpcTimedWait Call this something less generic? Even DoTransmitDataRpc() would work. PS16, Line 232: (timeout_ms == 0) ? 0 : if you just MonotonicMillis() + timeout_ms, and check for MonotonicMillis() >= deadline on line 235, you don't need the special case for timeout_ms == 0 I think. -- 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: 16 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: Huaisi Xu <h...@cloudera.com> Gerrit-Reviewer: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
