Michael Ho has posted comments on this change. Change subject: IMPALA-5388: Don't retry RPC calls on TSSLException ......................................................................
Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7063/1/be/src/runtime/client-cache.h File be/src/runtime/client-cache.h: PS1, Line 259: catch (const apache::thrift::TException& e) { : if (IsRecvTimeoutTException(e)) { : return Status(TErrorCode::RPC_RECV_TIMEOUT, strings::Substitute( : "Client $0 timed-out during recv call.", TNetworkAddressToString(address_))); : } : VLOG(1) << "client " << client_ << " unexpected exception: " : << e.what() << ", type=" << typeid(e).name(); : : // Client may have unexpectedly been closed, so re-open and retry. : // TODO: ThriftClient should return proper error codes. : const Status& status = Reopen(); : if (!status.ok()) { : if (retry_is_safe != NULL) *retry_is_safe = true; : return Status(TErrorCode::RPC_CLIENT_CONNECT_FAILURE, status.GetDetail()); : } : try { : (client_->*f)(*response, request); : } catch (apache::thrift::TException& e) { : // By this point the RPC really has failed. : // TODO: Revisit this logic later. It's possible that the new connection : // works but we hit timeout here. : return Status(TErrorCode::RPC_GENERAL_ERROR, e.what()); > The more I stare at this, the more I think it's broken even without SSL. It I agree. There is actually a TODO for it in the header comment. I didn't restructure the code in this change because that would be a more intrusive change and it's risky to restructure this heavily used path late into a release. -- To view, visit http://gerrit.cloudera.org:8080/7063 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I176975f2aa521d5be8a40de51067b1497923d09b Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
