[Impala-ASF-CR] IMPALA-5558: Reopen stale client connection

["Michael Ho (Code Review)" <ge...@cloudera.org>]

Michael Ho has uploaded a new patch set (#7).

Change subject: IMPALA-5558: Reopen stale client connection
......................................................................

IMPALA-5558: Reopen stale client connection

Previously, the retry logic in DoRpc() only allows retry to
happen if send() didn't complete successfully and the exception
indicates a closed connection. However, send() returning
successfully doesn't guarantee that the bytes have actually
reached the remote peer. It may still be buffered in the kernel
if there is room for it. TCP allows a connection to be half-open.
If an Impalad node is restarted, a stale client connection may
still allow send() to succeed  However, upon calling recv() in
the RPC call to fetch the response, the client will get a return
value of 0. In which case, thrift will throw an exception as the
connection to the remote peer is closed already. Apparently, the
existing retry logic doesn't quite handle this case. One can
consistently reproduce the problem by warming the client cache
followed by restarting one of the Impalad nodes. It will result
a series of query failures due to stale connections.

This change augments the retry logic to also retry the entire RPC
if the exception string contains the messages "No more data to read."
or "SSL_read: Connection reset by peer" to capture the case of stale
connections. Our usage of thrift doesn't involve half-open TCP connection
so having a broken connection in recv() indicates the remote end has
closed the socket already. The generated thrift code doesn't knowingly
close the socket before an RPC completes unless the process crashes,
the connection is stale (e.g. the remote node was restarted) or the
remote end fails to read from the client. In either cases, the entire
RPC should just be retried with a new connection.

This change also fixes QueryState::ReportExecStatusAux() to unconditionally
for up to 3 times when reporting exec status of a fragment instance. Previously,
it may break out of the loop early if RPC fails with 'retry_is_safe' == true
(e.g. due to stale connections above) or if the connection to coordinator fails.
Failing the RPC may cause all fragment instances of a query to be cncelled locally,
triggering query hang due to IMPALA-2990. The status reporting is idempotent as
the handler simply ignores profiles of fragment instances which are done already
so it should be retried uncontionally up to 3 times. Similarly, the cancellation
RPC is also idempotent so it should be unconditionally retried up to 3 times.

Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5
---
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/backend-client.h
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/query-state.cc
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/fault-injection-util.h
M tests/custom_cluster/test_rpc_exception.py
9 files changed, 88 insertions(+), 97 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/7284/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>