You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/06/20 06:58:55 UTC

[Impala-ASF-CR] IMPALA-5537: Retry RpcRecv on recv timeout exception for SSL connection

Michael Ho has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7229

Change subject: IMPALA-5537: Retry RpcRecv on recv timeout exception for SSL connection
......................................................................

IMPALA-5537: Retry RpcRecv on recv timeout exception for SSL connection

After the fix for IMPALA-5388, all TSSLException thrown will be
treated as fatal error and the query will fail. Turns out that
this is too strict and in a secure cluster under load, queries
can easily hit timeout waiting for RPC response.

When running without SSL, we call RetryRpcRecv() to retry the recv
part of an RPC if recv() called by the TSocket underlying the RPC
returns an EAGAIN. This change extends that logic to cover secure
connection. In particular, we pattern match against the exception
string "SSL_read: Resource temporarily unavailable" which corresponds
to EAGAIN error code being thrown in the SSL_read() path. The fault
injection utility has also been updated to distinguish between time
out and lost connection to exercise different error handling paths
in the send and recv paths.

After this change, we will still fail the RPC if there is
any error in the send part of a RPC with secure connection.

Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
---
M be/src/rpc/thrift-util.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
4 files changed, 63 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7229/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7229
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-5537: Retry RPC on somes exceptions with SSL connection

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change.

Change subject: IMPALA-5537: Retry RPC on somes exceptions with SSL connection
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7229/5/be/src/rpc/thrift-util.cc
File be/src/rpc/thrift-util.cc:

PS5, Line 198: This is not very robust as it's
             : // possible that the receiver of the RPC call may have received all the RPC payload
             : // but the ACK to the sender may have been dropped somehow. In which case, it's
             : // not safe to retry the RPC if it's not idempotent.
> maybe reword this to say the caller should first check if the send was comp
Done


PS5, Line 202: end-to-end tracking of RPC calls to detect duplicated calls in the receiver side
> what does that mean? is it still applicable?
Removed.


http://gerrit.cloudera.org:8080/#/c/7229/5/be/src/runtime/client-cache.h
File be/src/runtime/client-cache.h:

Line 330:     client_is_unrecoverable_ = false;
> i guess this restores the behavior before the patch that introduced this Re
Yes. It was missing in the last patch. It resulted in sub-optimal behavior as good connection may be closed.


-- 
To view, visit http://gerrit.cloudera.org:8080/7229
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5537: Retry RPC on somes exceptions with SSL connection

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5537: Retry RPC on somes exceptions with SSL connection
......................................................................


Patch Set 5: Code-Review+2

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7229/5/be/src/rpc/thrift-util.cc
File be/src/rpc/thrift-util.cc:

PS5, Line 198: This is not very robust as it's
             : // possible that the receiver of the RPC call may have received all the RPC payload
             : // but the ACK to the sender may have been dropped somehow. In which case, it's
             : // not safe to retry the RPC if it's not idempotent.
maybe reword this to say the caller should first check if the send was completed, or something? now that we do that.


PS5, Line 202: end-to-end tracking of RPC calls to detect duplicated calls in the receiver side
what does that mean? is it still applicable?


http://gerrit.cloudera.org:8080/#/c/7229/5/be/src/runtime/client-cache.h
File be/src/runtime/client-cache.h:

Line 330:     client_is_unrecoverable_ = false;
i guess this restores the behavior before the patch that introduced this RetryRpc() subroutine? okay.


-- 
To view, visit http://gerrit.cloudera.org:8080/7229
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5537: Retry RPC on somes exceptions with SSL connection

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Hello Henry Robinson, Sailesh Mukil, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7229

to look at the new patch set (#6).

Change subject: IMPALA-5537: Retry RPC on somes exceptions with SSL connection
......................................................................

IMPALA-5537: Retry RPC on somes exceptions with SSL connection

After the fix for IMPALA-5388, all TSSLException thrown will be
treated as fatal error and the query will fail. Turns out that
this is too strict and in a secure cluster under load, queries
can easily hit timeout waiting for RPC response.

When running without SSL, we call RetryRpcRecv() to retry the recv
part of an RPC if the TSocket underlying the RPC gets an EAGAIN
during recv(). This change extends that logic to cover secure
connection. In particular, we pattern match against the exception
string "SSL_read: Resource temporarily unavailable" which corresponds
to EAGAIN error code being thrown in the SSL_read() path.

Similarly, we will handle closed connection in send() path with
secure connection by pattern matching against the exception string
"TTransportException: Transport not open". To verify that the exception
is thrown during the send part of a RPC call, the RPC client interface
has been augmented to take a bool* argument which is set to true after
the send part of the RPC has completed but before the recv part starts.
If DoRPC() catches an exception and the send part isn't done yet, the
entire RPC if the exception string matches certain substrings which are
safe to retry.

The fault injection utility has also been updated to distinguish between
time out and lost connection to exercise different error handling paths
in the send and recv paths.

Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
---
A be/src/catalog/catalog-service-client-wrapper.h
M be/src/exec/catalog-op-executor.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/backend-client.h
M be/src/runtime/client-cache-types.h
M be/src/runtime/client-cache.h
M be/src/service/client-request-state.cc
A be/src/statestore/statestore-service-client-wrapper.h
A be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/fault-injection-util.h
M tests/custom_cluster/test_rpc_exception.py
17 files changed, 397 insertions(+), 92 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7229/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7229
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] IMPALA-5537: Retry RPC on somes exceptions with SSL connection

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5537: Retry RPC on somes exceptions with SSL connection
......................................................................


Patch Set 7:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/769/

-- 
To view, visit http://gerrit.cloudera.org:8080/7229
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5537: Retry RPC on somes exceptions with SSL connection

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5537: Retry RPC on somes exceptions with SSL connection
......................................................................


IMPALA-5537: Retry RPC on somes exceptions with SSL connection

After the fix for IMPALA-5388, all TSSLException thrown will be
treated as fatal error and the query will fail. Turns out that
this is too strict and in a secure cluster under load, queries
can easily hit timeout waiting for RPC response.

When running without SSL, we call RetryRpcRecv() to retry the recv
part of an RPC if the TSocket underlying the RPC gets an EAGAIN
during recv(). This change extends that logic to cover secure
connection. In particular, we pattern match against the exception
string "SSL_read: Resource temporarily unavailable" which corresponds
to EAGAIN error code being thrown in the SSL_read() path.

Similarly, we will handle closed connection in send() path with
secure connection by pattern matching against the exception string
"TTransportException: Transport not open". To verify that the exception
is thrown during the send part of a RPC call, the RPC client interface
has been augmented to take a bool* argument which is set to true after
the send part of the RPC has completed but before the recv part starts.
If DoRPC() catches an exception and the send part isn't done yet, the
entire RPC if the exception string matches certain substrings which are
safe to retry.

The fault injection utility has also been updated to distinguish between
time out and lost connection to exercise different error handling paths
in the send and recv paths.

Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
Reviewed-on: http://gerrit.cloudera.org:8080/7229
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
A be/src/catalog/catalog-service-client-wrapper.h
M be/src/exec/catalog-op-executor.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-util.cc
M be/src/runtime/backend-client.h
M be/src/runtime/client-cache-types.h
M be/src/runtime/client-cache.h
M be/src/service/client-request-state.cc
A be/src/statestore/statestore-service-client-wrapper.h
A be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M be/src/testutil/fault-injection-util.cc
M be/src/testutil/fault-injection-util.h
M tests/custom_cluster/test_rpc_exception.py
17 files changed, 425 insertions(+), 92 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7229
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I8243d4cac93c453e9396b0e24f41e147c8637b8c
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>