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

Posted to dev@impala.apache.org by "Juan Yu (Code Review)" <ge...@cloudera.org> on 2016/06/09 22:49:28 UTC

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Juan Yu has uploaded a new change for review.

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

Impala doesn't set socket send/recv timeout. RPC calls will wait
forever for data. In extreme case of network failure, or destination
host has kernel panic, sender will not get response and rpc call will
hang. Query hang is hard to detect. if hang happens at ExecRemoteFragment()
or CancelPlanFragments(), query cannot be canelled unless you restart
coordinator.

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread not quiting even after query is cancelled.

Besides the new EE test, I used the following iptable rule to
inject network failure to make sure rpc call never hang.
1. Block network traffic on a port completely
iptables -A INPUT -p tcp -m tcp --dport 22002 -j DROP
2. Randomly drop 5% of TCP packet to slowdown network
iptables -A INPUT -p tcp -m tcp --dport 22000 -m statistic --mode random --probability 0.05 -j DROP

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
8 files changed, 99 insertions(+), 29 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/2
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#11).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

Impala doesn't set socket send/recv timeout for backend client.
RPC calls will wait forever for data. In extreme case of bad network,
or destination host has kernel panic, sender will not get response
and rpc call will hang. Query hang is hard to detect. if hang happens
at ExecRemoteFragment() or CancelPlanFragments(), query cannot be
canelled unless you restart coordinator.

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread does not quiting even after query is cancelled.
For catalog client, keep default timeout to 0 (no timeout) because ExecDdl()
could take very long time if table has many partitons, mainly waiting for
HMS API call.

Added a new RPC call WaitRpcResp() to wait for receiver response for
longer time. This is needed by certain RPCs. For example, TransmitData()
by DataStreamSender, receiver could hold response due to back pressure.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
12 files changed, 219 insertions(+), 33 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/11
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 9:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3343/9//COMMIT_MSG
Commit Message:

PS9, Line 9: configable
configurable


PS9, Line 21: keep default timeout to 0
maybe mention in brackets that this means the timeouts are disabled.


Line 24: 
Also mention the WaitRpcResp()


PS9, Line 25: we don't put it back to cache
"we don't put the underlying connection back in the cache"


http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

Line 147:   shared_ptr<ThriftClientImpl> client_impl;
DCHECK(*client_key != NULL)


http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS9, Line 216: ONE_HOUR_IN_MS
How was one hour chosen? I'm worried this might cause lot of queries to hang for an hour and still end up failing.

Are we sure that TErrorCode::RPC_TIMEOUT only means that the timeout happened during recv()?


PS9, Line 306:   Status status = client.DoRpc(&ImpalaBackendClient::TransmitData, params, &res);
             :   if (status.code() == TErrorCode::RPC_TIMEOUT) {
             :     status = client.WaitRpcResp(&ImpalaBackendClient::recv_TransmitData, &res,
             :         runtime_state_, ONE_HOUR_IN_MS);
             :   }
             :   rpc_status_ = status;
This code seems to be reused in many places. Maybe you can create a wrapper function like DoRpcTimedWait() and call that here and everywhere else.


http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS9, Line 456: Note
"Note:"


-- 
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: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3343/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS1, Line 140: DEFINE_int32(catalog_client_send_timeout_ms, 0,
> will use a single one and will test if this affect large catalog, or a tabl
We do need a significant larger timeout for catalog connection.
when execute ddl query, if table has many partitions, HMS API call could take very long time, e.g. 10K partition could take 100s+
for 100K, it could take an hour


-- 
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

Added send/recv timeout to all rpc calls to avoid query hang.
For catalog client, keep default timeout to 0 (no timeout) because ExecDdl()
could take very long time if table has many partitons, mainly waiting for
HMS API call.

Added a new RPC call RetryRpcRecv() to wait for receiver response for
longer time. This is needed by certain RPCs. For example, TransmitData()
by DataStreamSender, receiver could hold response to add back pressure.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

Added retry for CancelPlanFragment RPC. This reduces the chance that cancel
request gets lost due to unstable network, but this can cause cancellation
takes longer time. and make test_lifecycle.py more flaky.
The metric num-fragments-in-flight might not be 0 yet due to previous tests.
Modified the test to check the metric delta instead of comparing to 0 to
reduce flakyness. However, this might not capture some failures.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/common/global-flags.cc
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/service/fragment-exec-state.cc
M be/src/service/fragment-mgr.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore.cc
A be/src/testutil/fault-injection-util.h
M be/src/util/error-util-test.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
18 files changed, 392 insertions(+), 53 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/17
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 17
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#9).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configable timeout for all backend client
RPC calls to avoid query hang issue.

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread does not quiting even after query is cancelled.
For catalog client, keep default timeout to 0 because ExecDdl() could take
very long time if table has many partitons, mainly waiting for HMS
API call.

If an RPC call fails, we don't put it back to cache but close it. This is to
make sure bad state in this connection won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
11 files changed, 200 insertions(+), 24 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/9
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/3343/4/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

Line 125: void ClientCacheHelper::ReleaseClient(ClientKey* client_key) {
> I think it would be better to reset the timeouts to the default values when
Good idea.
I am still running tests to see if we do need different timeout for different RPC calls.


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

PS4, Line 101: SetRpcTimeout
> Would this be clearer as SetClientRpcTimeout() here and below? On first loo
agreed


PS4, Line 239: SetRpcTimeout(send_timeout, recv_timeout);
> I'm a little worried about calling this every time we call a DoRpc() since 
You're right, set the timeout per client makes sense.


http://gerrit.cloudera.org:8080/#/c/3343/4/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS4, Line 556: is_cancelled_
> Can this also be IsCompleted()? Is it a possibility that there is a sequenc
StopReportThread is called if fragment is completed, or cancel happened during GetNext(). 
If an internal error happened at other time, it usually trigger internal cancelling, Cancel() will be called, 
But I agree, IsCompleted() seems a safer check here.


Line 559:     UpdateStatus(Status(TErrorCode::CANCELLED, "Fragment already cancelled."));
> Should this be an error? Or is it better to just return since the user will
I don't think this should be treat as error. the only issue is the report thread keep running and send status to coordinator which is confusing. 
I'll just call StopReportThread().


http://gerrit.cloudera.org:8080/#/c/3343/4/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS4, Line 128: cause problem
> If we know exactly what sort of problems it could cause, it would be worth 
UpdateFragmentExecStatus() will decrease num_remaining_fragment_instances_ counter, call it multiple times for a completed fragment will cause DCHECK.
I'll add those to comments.


http://gerrit.cloudera.org:8080/#/c/3343/4/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

PS4, Line 39: assert "RPC" in str(ex)
> 'ex' should contain "RPC timed out" right? This would pass for "RPC Error" 
We used to capture only one specific timeout case, and try reopen client for all other errors. I am not sure that's correct way so I modified the exception handling. But now I see more RPC error case than before. Maybe I should roll back the exception handling change.


-- 
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: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 17:

(18 comments)

Getting close, I like the way you've done the fault injection.

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

PS17, Line 203: if (client_ != NULL) {
              :       if (!has_rpc_error_) {
              :         client_cache_->ReleaseClient(&client_);
              :       } else {
              :         client_cache_->DestroyClient(&client_);
              :       }
              :     }
is this path covered by your tests - i.e. the connection will be reopened in the error case?


Line 301:   /// Indicate the rpc call sent by this connection succeeds or not. If the rpc call fails
last rpc call?


PS17, Line 304: has_rpc_error_
I would call this something more descriptive, like "cnxn_not_recoverable_".


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS17, Line 233: (timeout_ms == 0) ? 0 : 
can remove this, and just check timeout_ms in the while condition.


PS17, Line 236: deadline
timeout_ms


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS17, Line 116: can_retry
call this "retry_is_safe" ?


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:

PS17, Line 37: #ifndef NDEBUG
             :   DECLARE_int32(fault_injection_rpc_delay_ms);
             :   DECLARE_int32(fault_injection_rpc_type);
             :   #define FAULT_INJECTION_RPC_DALAY(type) InjectRpcDelay(type, \
             :       FLAGS_fault_injection_rpc_type, FLAGS_fault_injection_rpc_delay_ms)
             : #else
             :   #define FAULT_INJECTION_RPC_DALAY(type)
             : #endif
would it be better to put this in fault-injection-util.h?


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS17, Line 186: DALAY
DELAY


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS17, Line 15: _
nit: remove trailing _


http://gerrit.cloudera.org:8080/#/c/3343/17/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

PS17, Line 34: i
nit: use _ to show that we don't care about the variable


PS17, Line 61: test_runtime_filter_rpc
won't pytest try to run this as a test, since it's called test_*?


PS17, Line 72: 3000
is this definitely large enough to time out? I don't see if you verify that the query failed as expected - that seems important.


PS17, Line 72: -
just want to check this works as intended with a missing '-'


PS17, Line 73: rpc_timeout1
nit: can you call these "test_execplanfragment_timeout" etc?


http://gerrit.cloudera.org:8080/#/c/3343/17/tests/query_test/test_lifecycle.py
File tests/query_test/test_lifecycle.py:

PS17, Line 36: \
not needed


PS17, Line 52: \
remove


http://gerrit.cloudera.org:8080/#/c/3343/17/tests/verifiers/metric_verifier.py
File tests/verifiers/metric_verifier.py:

Line 61:   def __init__(self, impalad_service, metric_name):
please add a class comment saying what this is for


PS17, Line 68: wait_for_metric_reset
Is this class really necessary, or could we use MetricVerifier and always pass in init_value to wait_for_metric()?


-- 
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: 17
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3343/2//COMMIT_MSG
Commit Message:

PS2, Line 7: Add retry to backend connection request
Maybe add a brief line explaining the motivation behind this.


http://gerrit.cloudera.org:8080/#/c/3343/4/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

Line 125: void ClientCacheHelper::ReleaseClient(ClientKey* client_key) {
I think it would be better to reset the timeouts to the default values when we're releasing the client back to the cache. So that the next user of said cached client wouldn't have funky timeouts pre-set.

(This makes sense only if you choose to remove SetRpcTimeout() from DoRpc() and put it in the ClientConnection() constructor instead)


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

PS4, Line 101: SetRpcTimeout
Would this be clearer as SetClientRpcTimeout() here and below? On first look I thought it was set as the default for the entire client cache.


PS4, Line 239: SetRpcTimeout(send_timeout, recv_timeout);
I'm a little worried about calling this every time we call a DoRpc() since this method uses a lock. It would be better if we could avoid this.

One possibility is to set it during the creation of the ClientConnection where we can call SetRpcTimeout() only if it is created with explicit timeouts. Else the default will automatically be applied to that ThriftClientImpl.

So the ClientConnection() constructor signature will become something like this:
ClientConnection(ClientCache<T>* client_cache, TNetworkAddress address, Status* status, int32_t send_timeout = -1, int32_t recv_timeout = -1)

And right now I don't think we need the functionality of setting a RPC timeout per RPC vs per client. (correct me if I'm wrong)


http://gerrit.cloudera.org:8080/#/c/3343/4/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS4, Line 556: is_cancelled_
Can this also be IsCompleted()? Is it a possibility that there is a sequence of events where 'closed_' or 'done_' is set and this still ends up being called?


Line 559:     UpdateStatus(Status(TErrorCode::CANCELLED, "Fragment already cancelled."));
Should this be an error? Or is it better to just return since the user will already see that the query is cancelled.


http://gerrit.cloudera.org:8080/#/c/3343/4/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS4, Line 128: cause problem
If we know exactly what sort of problems it could cause, it would be worth briefly mentioning here.


http://gerrit.cloudera.org:8080/#/c/3343/4/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

PS4, Line 39: assert "RPC" in str(ex)
'ex' should contain "RPC timed out" right? This would pass for "RPC Error" as well.


-- 
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: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 10:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS10, Line 1495: rpc_status = backend_client.WaitRpcResp(
               :             &ImpalaBackendClient::recv_CancelPlanFragment, &res, runtime_state(), 100);
> By this stage, the send() part of the RPC has already succeeded right? So, 
yes, you're right. I like the idea of adding a retry flag.


http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS10, Line 230: Status DataStreamSender::Channel::DoRpcTimedWait(ImpalaBackendConnection* client,
              :     const TTransmitDataParams& params, TTransmitDataResult* res,
              :     uint64_t timeout) {
              :   Status status =
              :       client->DoRpc(&ImpalaBackendClient::TransmitData, params, res);
              :   if (status.code() == TErrorCode::RPC_TIMEOUT) {
              :     status = client->WaitRpcResp(&ImpalaBackendClient::recv_TransmitData, res,
              :         runtime_state_, ONE_HOUR_IN_MS);
              :   }
              :   return status;
              : }
> This would be more cleaner if done as a function in client-cache rather tha
Done


http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS10, Line 560: if (IsCompleted()) {
> What if 'done_' is true here?
when done_ is set, PlanFragmentExecutor::FragmentComplete() is always called and status is reported back to coordinator and report thread will be stopped.
check done_ here could accidentally stop report thread too soon and cause the fragment completed status not send to coordinator.


http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:

PS10, Line 146: IsCompleted()
> This technically should not be IsCompleted() because we check if (!done_). 
since it's only used once, I'll just check the vars directly instead of using a function.


-- 
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: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 15:

exhaustive test and stress test are green.
http://sandbox.jenkins.cloudera.com/view/Impala/view/Private-Utility/job/impala-private-build-and-test/3511/
http://sandbox.jenkins.cloudera.com/view/Impala/view/Stress/job/Impala-Stress-Test-Physical/615/

-- 
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: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 14: Code-Review+1

Carry Sailesh's +1

-- 
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: 14
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#5).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configable timeout for all backend client
RPC calls to avoid query hang issue.

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread does not quiting even after query is cancelled.
For catalog client, keep default timeout to 0 because ExecDdl() could take
very long time if table has many partitons, mainly waiting for HMS
API call.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/backend-client.h
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
11 files changed, 164 insertions(+), 23 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/5
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS9, Line 216: ONE_HOUR_IN_MS
> How was one hour chosen? I'm worried this might cause lot of queries to han
yes, that could happen. but we don't want very short timeout here. upstream operator sometimes could take very long time and I don't know how long it could be.
This is like the last mean to detect the pair node failure. In most of cases, the dest node failure will be detected by statestore heartbeat and the query will be cancelled before hitting this timeout.

RPC_TIMEOUT error is checked by IsTimeoutTException(), only if the error contains "EAGAIN (timed out)".
It happens only on TSocket::read()

--
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: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 16:

Just want to see if we can agree about how to handle the RPC call.
I'll continue working on adding tests.

-- 
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 <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 19:

(15 comments)

I think this looks good now. I'll keep thinking about it. 

Dan should do another round as well to give it the +2.

http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

PS19, Line 104: specify
nit: 'specifies'


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

PS19, Line 224: can_retry
update comment


PS19, Line 246: retry_is_safe
nit: be explicit here 

  if (retry_is_safe != NULL)


PS19, Line 277: RPC_RECV_TIMEOUT
and retry_is_safe == false, right? Otherwise we'll hang receiving data that will never show up.


PS19, Line 289: retry
retrying


Line 300:   /// Indicate the last rpc call sent by this connection succeeds or not. If the rpc call
nit: blank line before this one


PS19, Line 303: bool last_rpc_succeed_;
last_rpc_succeeded_

Still prefer "client_is_unrecoverable_" to more clearly describe the state, but don't feel so strongly about it.


PS19, Line 389: it's already bad
it's left in an unrecoverable state after errors in DoRpc()


http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS19, Line 154: i
nit: If


PS19, Line 154: infinitely
indefinitely


http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

PS19, Line 661: subscriber->id(), FLAGS_statestore_heartbeat_tcp_timeout_seconds));
why did you remove the subscriber address here? Is it because it's already in the error message?


http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS19, Line 16: FAULT_INJECTION__
not the same string as above


PS19, Line 41: int32_t
should these types be RpcCallType?


PS19, Line 41: inline
I don't think you have to mark this as inline. It only shows up in debug builds, and isn't very expensive.


http://gerrit.cloudera.org:8080/#/c/3343/19/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

PS19, Line 80:     
nit: indentation is off (should be two spaces, here and elsewhere)


-- 
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: 19
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 23: Verified+1

-- 
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: 23
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#10).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
11 files changed, 206 insertions(+), 25 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/10
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 23: Code-Review+2

Carry Dan's +2

-- 
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: 23
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 12:

(3 comments)

Could you also just briefly explain in the CR what the changes to test_lifecycle.py and metric_verifier.py are and why they were necessary?

http://gerrit.cloudera.org:8080/#/c/3343/12/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS12, Line 1494: NULL
Can't we pass runtime_state() here?


http://gerrit.cloudera.org:8080/#/c/3343/12/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS12, Line 120: NULL
Same here. Can't we pass executor_.runtime_state() here?


PS12, Line 122: Status(res.status)
Will this be valid when '!can_retry' is true?


-- 
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: 12
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 10:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS10, Line 1495: rpc_status = backend_client.WaitRpcResp(
               :             &ImpalaBackendClient::recv_CancelPlanFragment, &res, runtime_state(), 100);
By this stage, the send() part of the RPC has already succeeded right? So, if this call to WaitRpcResp() fails with an error other than RPC_TIMEOUT, wouldn't it loop back and call DoRpc() again, which means that now the RPC is sent twice?

Also, it would be useful to add a brief comment explaining why WaitRpcResp() is inside its own loop.


http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS10, Line 230: Status DataStreamSender::Channel::DoRpcTimedWait(ImpalaBackendConnection* client,
              :     const TTransmitDataParams& params, TTransmitDataResult* res,
              :     uint64_t timeout) {
              :   Status status =
              :       client->DoRpc(&ImpalaBackendClient::TransmitData, params, res);
              :   if (status.code() == TErrorCode::RPC_TIMEOUT) {
              :     status = client->WaitRpcResp(&ImpalaBackendClient::recv_TransmitData, res,
              :         runtime_state_, ONE_HOUR_IN_MS);
              :   }
              :   return status;
              : }
This would be more cleaner if done as a function in client-cache rather than over here. So even the code in the coordinator and the fragment-exec-state can call this function.

You could add a retry parameter to the function signature as well.


http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS10, Line 560: if (IsCompleted()) {
What if 'done_' is true here?


http://gerrit.cloudera.org:8080/#/c/3343/10/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:

PS10, Line 146: IsCompleted()
This technically should not be IsCompleted() because we check if (!done_). Maybe think of a different name?


-- 
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: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Henry Robinson, Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC to avoid query hang issue.

Prior to this change, Impala doesn't set socket send/recv timeout for
backend client. RPC will wait forever for data. In extreme cases
of bad network or destination host has kernel panic, sender will not
get response and RPC will hang. Query hang is hard to detect. If
hang happens at ExecRemoteFragment() or CancelPlanFragments(), query
cannot be canelled unless you restart coordinator.

Added send/recv timeout to all RPCs to avoid query hang. For catalog
client, keep default timeout to 0 (no timeout) because ExecDdl()
could take very long time if table has many partitons, mainly waiting
for HMS API call.

Added a wrapper RetryRpcRecv() to wait for receiver response for
longer time. This is needed by certain RPCs. For example, TransmitData()
by DataStreamSender, receiver could hold response to add back pressure.

If an RPC fails, the connection is left in an unrecoverable state.
we don't put the underlying connection back to cache but close it. This
is to make sure broken connection won't cause more RPC failure.

Besides the new EE test, I used the following iptables rule to
inject network failure to verify RPCs never hang.
1. Block network traffic on a port completely
iptables -A INPUT -p tcp -m tcp --dport 22002 -j DROP
2. Randomly drop 5% of TCP packets to slowdown network
iptables -A INPUT -p tcp -m tcp --dport 22000 -m statistic --mode random --probability 0.05 -j DROP

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/common/global-flags.cc
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/service/fragment-exec-state.cc
M be/src/service/impala-internal-service.h
M be/src/statestore/statestore.cc
A be/src/testutil/fault-injection-util.h
M be/src/util/error-util-test.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
17 files changed, 397 insertions(+), 64 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/22
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 22
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

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 <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, the connection is left in an unrecoverable state.
we don't put the underlying connection back to cache but close it. This
is to make sure broken connection won't cause more RPC failure.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/20
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/19
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 19
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 19:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

PS19, Line 104: specify
> nit: 'specifies'
Done


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

PS19, Line 224: can_retry
> update comment
Done


PS19, Line 246: retry_is_safe
> nit: be explicit here 
Done


PS19, Line 277: RPC_RECV_TIMEOUT
> and retry_is_safe == false, right? Otherwise we'll hang receiving data that
TIMEOUT indicates retry_is_safe = false.
if DoRpc() return TIMEOUT error, it always sets and retry_is_safe = false, see L#246. 
Do we need to explicitly mention it?


PS19, Line 289: retry
> retrying
Done


Line 300:   /// Indicate the last rpc call sent by this connection succeeds or not. If the rpc call
> nit: blank line before this one
Done


PS19, Line 303: bool last_rpc_succeed_;
> last_rpc_succeeded_
Done


PS19, Line 389: it's already bad
> it's left in an unrecoverable state after errors in DoRpc()
Done


http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS19, Line 154: infinitely
> indefinitely
Done


PS19, Line 154: i
> nit: If
Done


http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

PS19, Line 661: subscriber->id(), FLAGS_statestore_heartbeat_tcp_timeout_seconds));
> why did you remove the subscriber address here? Is it because it's already 
yes.


http://gerrit.cloudera.org:8080/#/c/3343/19/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS19, Line 16: FAULT_INJECTION__
> not the same string as above
Done


PS19, Line 41: inline
> I don't think you have to mark this as inline. It only shows up in debug bu
Done


PS19, Line 41: int32_t
> should these types be RpcCallType?
They should be. but the config setting type can only be int32 though.


http://gerrit.cloudera.org:8080/#/c/3343/19/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

PS19, Line 80:     
> nit: indentation is off (should be two spaces, here and elsewhere)
Done


-- 
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: 19
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#7).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configable timeout for all backend client
RPC calls to avoid query hang issue.

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread does not quiting even after query is cancelled.
For catalog client, keep default timeout to 0 because ExecDdl() could take
very long time if table has many partitons, mainly waiting for HMS
API call.

If an RPC call fails, we don't put it back to cache but close it. This is to
make sure bad state in this connection won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
12 files changed, 205 insertions(+), 27 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 20:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/3343/20//COMMIT_MSG
Commit Message:

Line 12: Impala doesn't set socket send/recv timeout for backend client.
> Prior to this change, ...
Done


PS20, Line 24: new RPC call RetryRpcRecv()
> This makes it sound like RetryRpcRecv() is a new RPC.  I haven't looked at 
Done


PS20, Line 28: call
> delete (C of RPC stands for call)
Done


http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

PS20, Line 102: call
> delete
Done


PS20, Line 102: which trigger caller hits RPC timeout.
> to trigger an RPC timeout on the caller side.
Done


http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

Line 162:   client_map_.erase(client);
> is it legal to use the 'client' iterator even though the lock was dropped i
http://kera.name/articles/2011/06/iterator-invalidation-rules/
For insert, iterator is not affected. For Erasure, iterator is only affected to the erased elements.
client is not shared so there shouldn't be more than one thread try to destroy the same client. right?


Line 163:   *client_key = NULL;
> why doesn't this method need to remove from the per_host_caches_ as well? T
Connection cannot be shared. when a connection is being use, it was taken out from per_host_caches. so we just don't put it back.
The header comment means client_map_. I'll clarify that.


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

PS20, Line 96: it
> what is "it"? the client? or the connection?  does this NULL out *client_ke
"it" means the client.  DestroyClient NULL out *client_key like Release Client()


PS20, Line 96: cache
> is this "the per-host cache" that is referenced everywhere else, or somethi
Done


PS20, Line 224: indicate
> indicates
if returned status is okay, means both RPC are completed. can or cannot retry really depends on the data being sent here.
If this is TransmiteData, retry is not safe
but if this is ReportStatusCb, retry is fine.
Only caller knows that info.


Line 225:   /// the whole RPC call.
> explain this more.  is it because if the send RPC call failed, then the han
that's right.


Line 260:       if (!status.ok()) {
> so this is the only path where we have *retry_is_safe == true and an error 
Done


Line 265:       } catch (apache::thrift::TException& e) {
> why does this path not need the timeout check also?
I think the original logic is to give it a second chance but if it fails again, just fail the whole RPC.


Line 271:     client_is_unrecoverable_ = false;
> why is this logic needed now, whereas it wasn't needed before? or is this a
yes, it's another bug fix, I need to fix it in this patch, otherwise, the broken connection will cause lots of problem.
IMPALA-2864


PS20, Line 301: the
> whether the
see my previous patch and Henry's comment
"last_rpc_succeeded_" matches the code logic better.
"client_is_unrecoverable_" to more clearly describe the state
not sure which one is better.


http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

Line 115:   ImpalaBackendClientCache* client_cache_;
> now that we store the RuntimeState, let's get rid of this and just get the 
Done


PS20, Line 153: wait
> retry waiting for the response?
Done


PS20, Line 153: call
> delete "call", and would be nice to write RPC
Done


PS20, Line 231: timeout_ms
> where is this called with a non-zero timeout_ms?
For now, we like to wait indefinitely to avoid logic change.
But in the future, even after replace RPC implementation, it's better to have a reasonable timeout for all RPCs.


Line 239:               timeout_ms));
> this will throw away the original status message.  Maybe use AddDetail() in
Done


http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

Line 125:     if (!retry_is_safe) break;
> so i guess this was a bug in the old code?
yes, But for ReportExecStatus RPC, resend shouldn't cause any issue. handler just process the same status multiple times.


http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

PS20, Line 658: Rewrite
> Add details to
Done


PS20, Line 669: Rewrite
> same
Done


-- 
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: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 11:

(2 comments)

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

PS11, Line 272: bool* can_retry = NULL
Would it be easy to just pass a 'int num_retry' here, so that the retry loop can go within this function?
That would mean less function calls and also one less pointer dereference.
If it's not straightforward to do so, then this is fine.


PS11, Line 281: //status = WaitRpcResp(wait_func, response, state, timeout);
Is this logic complete? Or will you be posting another patch?


-- 
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: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 11:

(2 comments)

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

PS11, Line 272: bool* can_retry = NULL
> Would it be easy to just pass a 'int num_retry' here, so that the retry loo
I am afraid that way this function does too much.
I prefer to leave the retry to caller, so caller can decide to use the same connection or a new connection, or adding some delay between retry.


PS11, Line 281: //status = WaitRpcResp(wait_func, response, state, timeout);
> Is this logic complete? Or will you be posting another patch?
sorry forget to remove the old code.
and I do need to post a new patch. just found that RuntimeState might not always be available.


-- 
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: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M be/src/util/error-util-test.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
14 files changed, 237 insertions(+), 53 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/16
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 16
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC to avoid query hang issue.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Reviewed-on: http://gerrit.cloudera.org:8080/3343
Reviewed-by: Juan Yu <jy...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/common/global-flags.cc
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/service/fragment-exec-state.cc
M be/src/service/impala-internal-service.h
M be/src/statestore/statestore.cc
A be/src/testutil/fault-injection-util.h
M be/src/util/error-util-test.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
17 files changed, 397 insertions(+), 64 deletions(-)

Approvals:
Juan Yu: Looks good to me, approved
Internal Jenkins: Verified

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 24
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 20:

were you able to verify how many of the RPCs you were able to exercise timeouts by using the iptables thing with the stress test?

-- 
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: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 15:

(11 comments)

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

Line 97:   void DestroyClient(ClientKey* client_key);
> needs a comment
Done


PS15, Line 197: hasRpcError_
> C++-style naming
Done


PS15, Line 239: IsTimeoutTException
> should now be called IsRecvTimeoutTException()
Done


PS15, Line 240: RPC_TIMEOUT
> should be RPC_RECV_TIMEOUT
Done


PS15, Line 246: if (strstr(e.what(),"unknown result") != NULL)
> this seems very brittle, and at least should be wrapped in an IsXXXExceptio
Done


PS15, Line 268: release
> back pressure is added by a blocking RPC, not released.
Done


PS15, Line 273: timeout
> what's the unit?
Done


PS15, Line 272: Status DoRpcTimedWait(const F& f, const Request& request, Response* response,
              :       const G& wait_func, uint64_t timeout, RuntimeState* state, bool* can_retry = NULL) 
> This seems like it breaks abstractions: not all RPCs happen in the context 
I agree with you. I did it this way in patch#6. but had to duplicate the wait response loop for several rpc calls so I changed it later.


PS15, Line 282: bool no_timeout = timeout == 0;
> this can be simplified by having a deadline variable:
Done


Line 290:           if (!IsTimeoutTException(e)) {
> Just curious, which function in recv_TransmitData() throws timeout exceptio
The exception is thrown by thrift, see TSocket.cpp read()
recv_TransmitData() will call thrift api to read data from server side.


PS15, Line 309: bool hasRpcError_;
> the role of this variable is not clear. Please add a comment.
Done


-- 
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: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 12:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3343/12/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS12, Line 1494: NULL
> Can't we pass runtime_state() here?
We can, but I don't think we need to check it. even if the query is cancelled, we still want to send the cancel request to remote node.
And this one has a very short timeout, the rpc won't hang for long time like the one in TransmitData().


http://gerrit.cloudera.org:8080/#/c/3343/12/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS12, Line 120: NULL
> Same here. Can't we pass executor_.runtime_state() here?
see L#93, runtime state may not have been set.
and same as CancelRemoteFragment request, this one only has short timeout, it won't hang for long.


PS12, Line 122: Status(res.status)
> Will this be valid when '!can_retry' is true?
Good catch, I'll fix it.


-- 
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: 12
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.

Huaisi Xu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 15:

(1 comment)

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

Line 290:           if (!IsTimeoutTException(e)) {
Just curious, which function in recv_TransmitData() throws timeout exception? I could not find one?


-- 
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: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#3).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread not quiting even after query is cancelled.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/3
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 16:

(1 comment)

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 232: (timeout_ms == 0) ? 0 : 
> timeout == 0 means wait infinitely.
oh right, of course.


-- 
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 <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 1:

(2 comments)

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

PS1, Line 242: e.getType() == apache::thrift::transport::TTransportException::TIMED_OUT
> This IsTimeoutTException checks error string "EAGAIN (timed out)", this onl
Sorry I misunderstand the timeout handling here. I'll revert the exception handling here.
Correct me if I am wrong
We want to return error is timeout happens at recv call which means send is done and error or exception at thrift server side. and we want to retry the whole RPC for all the rest scenarios (mainly resource unavailable cases)


http://gerrit.cloudera.org:8080/#/c/3343/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS1, Line 134: DEFINE_int32(backend_client_send_timeout_ms, 1000, "(Advanced) The time after "
             :     "which a backend client send RPC call will timeout.");
> we can just use a single one
According to my testing, a single receive timeout might not be sufficient.
If sending timeout, we might want to fail the query or retry the whole RPC, especially for control messages, like CancelPlanFragment
If recv timeout, we should just return error. But we don't want error out accidentally, e.g. data stream sender might need to wait for long time due to back pressure.
I am going to add a loop for recv_TransmitData() to make sure we wait long enough.
Does this make sense?


-- 
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: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 21:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3343/21/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS21, Line 44: rpc_type == RPC_NULL
> Okay, but then this condition isn't needed since line 46 will never be true
Done


-- 
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: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 16:

(6 comments)

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"
Done


Line 254:           << e.what() << ", type=" << typeid(e).name();
> nit: align << with above
Done


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 re
Done


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?
As discussed offline, I don't want to change destroy the connection right away because the retry logic in DoRpc.


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.
Done


PS16, Line 232: (timeout_ms == 0) ? 0 : 
> if you just MonotonicMillis() + timeout_ms, and check for MonotonicMillis()
timeout == 0 means wait infinitely.
I add comments for that.


-- 
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 <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 15:

(11 comments)

How are you going to test all these failure paths for all the RPCs we have in the system? How do we convince ourselves that these failures correctly lead to query tear down in all cases?

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

Line 97:   void DestroyClient(ClientKey* client_key);
needs a comment


PS15, Line 197: hasRpcError_
C++-style naming


PS15, Line 239: IsTimeoutTException
should now be called IsRecvTimeoutTException()


PS15, Line 240: RPC_TIMEOUT
should be RPC_RECV_TIMEOUT


PS15, Line 246: if (strstr(e.what(),"unknown result") != NULL)
this seems very brittle, and at least should be wrapped in an IsXXXException() kind of method.


PS15, Line 268: release
back pressure is added by a blocking RPC, not released.


PS15, Line 273: timeout
what's the unit?


PS15, Line 272: Status DoRpcTimedWait(const F& f, const Request& request, Response* response,
              :       const G& wait_func, uint64_t timeout, RuntimeState* state, bool* can_retry = NULL) 
This seems like it breaks abstractions: not all RPCs happen in the context of a query. I also don't know what wait_func is for, based on the comments.

I think this might be better written as two methods, which would be called like this:

  Status status = client->DoRpc(...., &can_retry);
  while (status.code() == TErrorCode::RPC_TIMEOUT && can_retry) {
    check_if_cancelled_or_timeout();
    status = client->RetryRpcRecv(recv_func);
  }

I understand the idea behind pushing all this logic into this method, but I think it actually hides too much from the caller. This allows the caller to take appropriate action on network timeouts without handing in a runtime state to the client (e.g. the caller may want to do some more work before blocking again).

The other thing you could do is pass in a callback, so we'd have:

 Status status = client->DoRpc(...., [state]() { return state->is_cancelled(); }, ...);

but my fear is that would get a bit messy.


PS15, Line 282: bool no_timeout = timeout == 0;
this can be simplified by having a deadline variable:

  uint64_t deadline = MonotonicMillis() + timeout;


PS15, Line 309: bool hasRpcError_;
the role of this variable is not clear. Please add a comment.


http://gerrit.cloudera.org:8080/#/c/3343/15/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS15, Line 1490: // Try to send the RPC 3 times before failing.
Why try 3 times? Have you seen in your testing where there's failure on the first try, but success on attempts 2 or 3?


-- 
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: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

Added a new RPC call DoRpcTimedWait() to wait for receiver response for
longer time. This is needed by certain RPCs. For example, TransmitData()
by DataStreamSender, receiver could hold response due to back pressure.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/14
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 14
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/18
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 18
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
11 files changed, 205 insertions(+), 30 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/15
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 22:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3343/21/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS21, Line 44:  void InjectRpcDelay
> Just for easy testing, you can easily enable disable the fault injection by
Okay, but then this condition isn't needed since line 46 will never be true, right? (my_type is never RPC_NULL)


-- 
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: 22
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

Patch Set 15:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3343/15/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS15, Line 1490: // Try to send the RPC 3 times before failing.
> Why try 3 times? Have you seen in your testing where there's failure on the
This is to increase the chance the cancel request can reach remote nodes to avoid orphan fragments. If network is not stable, we could get "send expire" error on the coordinator to remote node connection, but the report status callback might keep working so remote nodes don't aware there is connection issue with coordinator.
Though DoRpc() will always retry once, in the situation of connection storm, it might not be able to create a new connection at first retry.
If you are unlucky, you could get a closed connection from cache (this could happen if CreateClient() in ClientCacheHelper::ReopenClient() fails for previous RPC call). then the cancel request might not get a chance to send out.

--
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: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#6).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configable timeout for all backend client
RPC calls to avoid query hang issue.

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread does not quiting even after query is cancelled.
For catalog client, keep default timeout to 0 because ExecDdl() could take
very long time if table has many partitons, mainly waiting for HMS
API call.

If an RPC call fails, we don't put it back to cache but close it. This is to
make sure bad state in this connection won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
12 files changed, 203 insertions(+), 27 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 20:

Most one that hit RPC timeout are TransmitData and ReportStatusCb.
The EE test make sure every RPC is tested.

-- 
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: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Hello Henry Robinson, Sailesh Mukil,

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

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

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC to avoid query hang issue.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/common/global-flags.cc
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/service/fragment-exec-state.cc
M be/src/service/impala-internal-service.h
M be/src/statestore/statestore.cc
A be/src/testutil/fault-injection-util.h
M be/src/util/error-util-test.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
17 files changed, 383 insertions(+), 64 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/21
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 21:

(15 comments)

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

PS21, Line 227: is
> delete "is"
Done


Line 304:   TNetworkAddress address_;
> can't we get this from client_->address()?
"client_" is not an instance of ThriftClientImpl


http://gerrit.cloudera.org:8080/#/c/3343/21/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS21, Line 134: 300000
> Is there a short comment you could write to justify how this was chosen (5 
Done


PS21, Line 134: The time after "
              :     "which a backend client send/recv RPC call will timeout.
> The send/recv connection timeout in milliseconds for a backend client RPC.
This is the underlying TSocket send/recv call timeout, not connection timeout.


PS21, Line 138:  
> same
Done


PS21, Line 157: 0
> why is this 0? (wait_ms)
This is for retry opening connection, usually each retry will take several seconds. waiting even longer won't help much.


PS21, Line 162: 100
> how was this chosen?
I'll set this to 0.


Line 223:             "", !FLAGS_ssl_client_ca_certificate.empty())),
> not your change, but it's really unfortunate we duplicate this code. let's 
I'll add a Todo here.


http://gerrit.cloudera.org:8080/#/c/3343/21/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS21, Line 36: RPC_RANDOM
> comment that this must be last
Done


PS21, Line 39: call
> delete
Done


PS21, Line 40: timeout
> this is the recv connection timeout, correct? if so, how about saying "recv
Done


PS21, Line 41: RpcCallType my_type, int32_t rpc_type, int32_t delay_ms
> document these.
Done


PS21, Line 44: rpc_type == RPC_NULL
> what is specifying RPC_NULL used for?
Just for easy testing, you can easily enable disable the fault injection by changing the value, no need to add/remove the startup flag. In the future, we could change this value dynamically to do more testing.


Line 50:       FLAGS_fault_injection_rpc_type, FLAGS_fault_injection_rpc_delay_ms)
> why pass these as arguments rather than just having InjectRpcDelay() read t
Similar reason as above, we could test with dynamic values without the need to restart cluster.


http://gerrit.cloudera.org:8080/#/c/3343/21/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

Line 119:     self.execute_query_verify_metrics(self.TEST_QUERY, 10)
> how long do all these tests take to execute?  let's run them only in exhaus
About 5 minutes. ok, I'll change to only execute in exhaustive mode.


-- 
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: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

Patch Set 20:

(5 comments)

Do you have a new patchset I can look at?

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

Line 162: client_map_.erase(client);
> http://kera.name/articles/2011/06/iterator-invalidation-rules/
Yeah, the iterator is still valid since no other thread would have removed it. And I missed that you do retake the lock to protect erase(), so agree we are okay.

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

PS20, Line 224: indicate
> if returned status is okay, means both RPC are completed. can or cannot ret
Right, but in the case of success, it's not really a "retry". That's just saying whether the RPC is idempotent or not.

I'm more talking about whether we guarantee that retry_is_safe is always set regardless of the return value, or if we only set it for errors. The words "the failure" makes it ambigous. How about saying:

In case of error, '*retry_is_safe' is set to true if it's safe to retry the RPC because the send never occurred. Otherwise, it's set to false to indicate that the RPC was in progress when it failed, and therefore retrying the RPC is not safe.

Line 265: } catch (apache::thrift::TException& e) {
> I think the original logic is to give it a second chance but if it fails ag
I'm fine with leaving this alone for now but it seems a little weird.

PS20, Line 301: the
> see my previous patch and Henry's comment
I'm fine with either, so if it was already discussed and this is preferred, okay to leave it alone.

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS20, Line 231: timeout_ms
> For now, we like to wait indefinitely to avoid logic change.
It'd be preferred to remove the timeout_ms param and this extra logic until we actually start using it, since having it in the code base implies it's being tested, but it's not. Also it's confusing to have dead code, everyone's going to wonder if this is intentional or not (as was my question). So, would you mind moving this into the "future" patch that sets non-zero timeouts?

--
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: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 21:

(15 comments)

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

PS21, Line 227: is
delete "is"


Line 304:   TNetworkAddress address_;
can't we get this from client_->address()?


http://gerrit.cloudera.org:8080/#/c/3343/21/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS21, Line 134: 300000
Is there a short comment you could write to justify how this was chosen (5 minutes)?


PS21, Line 134: The time after "
              :     "which a backend client send/recv RPC call will timeout.
The send/recv connection timeout in milliseconds for a backend client RPC.

(because it's a timeout, not a time, and to be clear it's the connection timeout, not the call timeout).


PS21, Line 138:  
same


PS21, Line 157: 0
why is this 0? (wait_ms)


PS21, Line 162: 100
how was this chosen?


Line 223:             "", !FLAGS_ssl_client_ca_certificate.empty())),
not your change, but it's really unfortunate we duplicate this code. let's deal with that later, though.


http://gerrit.cloudera.org:8080/#/c/3343/21/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS21, Line 36: RPC_RANDOM
comment that this must be last


PS21, Line 39: call
delete


PS21, Line 40: timeout
this is the recv connection timeout, correct? if so, how about saying "recv timeout" to be more explicit.


PS21, Line 41: RpcCallType my_type, int32_t rpc_type, int32_t delay_ms
document these.


PS21, Line 44: rpc_type == RPC_NULL
what is specifying RPC_NULL used for?


Line 50:       FLAGS_fault_injection_rpc_type, FLAGS_fault_injection_rpc_delay_ms)
why pass these as arguments rather than just having InjectRpcDelay() read them directly?  Do we ever pass other argument values to this function?


http://gerrit.cloudera.org:8080/#/c/3343/21/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

Line 119:     self.execute_query_verify_metrics(self.TEST_QUERY, 10)
how long do all these tests take to execute?  let's run them only in exhaustive if we aren't already.


-- 
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: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 9:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/3343/9//COMMIT_MSG
Commit Message:

PS9, Line 9: configable
> configurable
Done


PS9, Line 21: keep default timeout to 0
> maybe mention in brackets that this means the timeouts are disabled.
Done


Line 24: 
> Also mention the WaitRpcResp()
Done


PS9, Line 25: we don't put it back to cache
> "we don't put the underlying connection back in the cache"
Done


http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

Line 147:   shared_ptr<ThriftClientImpl> client_impl;
> DCHECK(*client_key != NULL)
Done


http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS9, Line 306:   Status status = client.DoRpc(&ImpalaBackendClient::TransmitData, params, &res);
             :   if (status.code() == TErrorCode::RPC_TIMEOUT) {
             :     status = client.WaitRpcResp(&ImpalaBackendClient::recv_TransmitData, &res,
             :         runtime_state_, ONE_HOUR_IN_MS);
             :   }
             :   rpc_status_ = status;
> This code seems to be reused in many places. Maybe you can create a wrapper
Done


http://gerrit.cloudera.org:8080/#/c/3343/9/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS9, Line 456: Note
> "Note:"
Done


-- 
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: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#8).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configable timeout for all backend client
RPC calls to avoid query hang issue.

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread does not quiting even after query is cancelled.
For catalog client, keep default timeout to 0 because ExecDdl() could take
very long time if table has many partitons, mainly waiting for HMS
API call.

If an RPC call fails, we don't put it back to cache but close it. This is to
make sure bad state in this connection won't cause more RPC failure.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/service/fragment-exec-state.cc
M be/src/statestore/statestore.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
12 files changed, 204 insertions(+), 27 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/8
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 20:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:

Line 38:   FAULT_INJECTION_RPC_DELAY(RPC_EXECPLANFRAGMENT);
> is there a reason we decided to put these here rather than in impala-intern
No, I forget we have a proxy for those RPC call. You are right, it's better to put them there.


-- 
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: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 5:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3343/5//COMMIT_MSG
Commit Message:

PS5, Line 17: unless you restart coordinato
This is true if the remote peer does not send an RST (which would be the case with a kernel panic or network failure). We could consider using per-connection TCP keepalives to address this in the future.


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 combine that with per-client timeouts, if you think that's neater, but this custom logic is best avoided if possible.

The preferred way to do this is to have TransmitData() return "not ready" as a status, and then have the client retry with an application-level timeout.


PS5, Line 51: MonotonicNanos
nit: use MontonicMillis()


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.


Line 1492:       rpc_status= backend_client.DoRpc(
space before '='


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 RPC three times? What error condition does the retry address?


http://gerrit.cloudera.org:8080/#/c/3343/5/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS5, Line 127: // Otherwise Coordinator::UpdateFragmentExecStatus() could be called multiple times
             :     // for the same finished PlanFragmentExecutor, num_remaining_fragment_instances_ will
             :     // be decreased more than once and cause DCHECK.
We should fix the bug in UpdateFragmentExecStatus(), rather than trying to avoid it here. If the first attempt didn't get through, but the executor_.IsCompleted() is true, won't that mean the status will never be sent?


PS5, Line 138: // TODO: Do we really need to cancel?
I think we definitely should cancel: this makes us think the coordinator has failed.


-- 
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 <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 17:

(17 comments)

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

PS17, Line 203: if (client_ != NULL) {
              :       if (!has_rpc_error_) {
              :         client_cache_->ReleaseClient(&client_);
              :       } else {
              :         client_cache_->DestroyClient(&client_);
              :       }
              :     }
> is this path covered by your tests - i.e. the connection will be reopened i
yes, the test_random_rpc_timeout covers this. It runs multiple queries and each will hit rpc timeout, then the connection will be destroyed. The following query will create new connections.


Line 301:   /// Indicate the rpc call sent by this connection succeeds or not. If the rpc call fails
> last rpc call?
Done


PS17, Line 304: has_rpc_error_
> I would call this something more descriptive, like "cnxn_not_recoverable_".
How about just "last_rpc_succeed_" so it matches the logic, less confusion.


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS17, Line 233: (timeout_ms == 0) ? 0 : 
> can remove this, and just check timeout_ms in the while condition.
Done


PS17, Line 236: deadline
> timeout_ms
Done


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

PS17, Line 116: can_retry
> call this "retry_is_safe" ?
Done


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:

PS17, Line 37: #ifndef NDEBUG
             :   DECLARE_int32(fault_injection_rpc_delay_ms);
             :   DECLARE_int32(fault_injection_rpc_type);
             :   #define FAULT_INJECTION_RPC_DALAY(type) InjectRpcDelay(type, \
             :       FLAGS_fault_injection_rpc_type, FLAGS_fault_injection_rpc_delay_ms)
             : #else
             :   #define FAULT_INJECTION_RPC_DALAY(type)
             : #endif
> would it be better to put this in fault-injection-util.h?
Figured out the compiling issue.
Yes, better to put this in fault-injection-util.h and avoid duplicate code.


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS17, Line 186: DALAY
> DELAY
Done


http://gerrit.cloudera.org:8080/#/c/3343/17/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS17, Line 15: _
> nit: remove trailing _
Done


http://gerrit.cloudera.org:8080/#/c/3343/17/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:

PS17, Line 34: i
> nit: use _ to show that we don't care about the variable
Done


PS17, Line 61: test_runtime_filter_rpc
> won't pytest try to run this as a test, since it's called test_*?
it does. I'll change the name.


PS17, Line 72: 3000
> is this definitely large enough to time out? I don't see if you verify that
yes, since backend_client_rpc_timeout_ms is set to 1000.
I verified log each RPC call did hit the timeout. however hit timeout doesn't necessary lead to query completely failure. It trigger fragment cancel, Cancel() just set runtime state cancel flag, doesn't force query execution stop. by the time execNode check cancel flag, the execution could be already complete and query finish successfully. Also some rpc failure not affect query execution, like the runtime filter one.
The only one can guarantee query failure is the ExecPlanFragment(), I'll check query failure for it.


PS17, Line 72: -
> just want to check this works as intended with a missing '-'
yes, both "-" and "--" work. but I'll make them consistent.


PS17, Line 73: rpc_timeout1
> nit: can you call these "test_execplanfragment_timeout" etc?
Done


http://gerrit.cloudera.org:8080/#/c/3343/17/tests/query_test/test_lifecycle.py
File tests/query_test/test_lifecycle.py:

PS17, Line 36: \
> not needed
Done


PS17, Line 52: \
> remove
Done


http://gerrit.cloudera.org:8080/#/c/3343/17/tests/verifiers/metric_verifier.py
File tests/verifiers/metric_verifier.py:

PS17, Line 68: wait_for_metric_reset
> Is this class really necessary, or could we use MetricVerifier and always p
we usually need to check metrics for all impalad. I'll modify MetricVerifier to keep initial values.


-- 
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: 17
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#13).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/13
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 22: Code-Review+2

-- 
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: 22
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 20: Code-Review+1

-- 
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: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 13: Code-Review+1

(1 comment)

I think you missed this the last time:
"Could you also just briefly explain in the CR what the changes to test_lifecycle.py and metric_verifier.py are and why they were necessary?"

Otherwise, this looks fine to me now.

http://gerrit.cloudera.org:8080/#/c/3343/13/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS13, Line 560: (!done_ && (closed_ || is_cancelled_))
It would be useful to add a comment here saying that done_ should not be true or else we might stop the report thread while it's sending the final report.


-- 
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: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 13:

(1 comment)

I added a section in commit message. Is it enough for explain the test change?
"Added retry for CancelPlanFragment RPC. This reduces the chance that cancel
request gets lost due to unstable network, but this can cause cancellation
takes longer time. and make test_lifecycle.py more flaky.
The metric num-fragments-in-flight might not be 0 yet due to previous tests.
Modified the test to check the metric delta instead of comparing to 0 to
reduce flakyness. However, this might not capture some failures.
"

http://gerrit.cloudera.org:8080/#/c/3343/13/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS13, Line 560: (!done_ && (closed_ || is_cancelled_))
> It would be useful to add a comment here saying that done_ should not be tr
will do.


-- 
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: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (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/3343

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

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC to avoid query hang issue.

Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
---
M be/src/common/global-flags.cc
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/exec-env.cc
M be/src/service/fragment-exec-state.cc
M be/src/service/impala-internal-service.h
M be/src/statestore/statestore.cc
A be/src/testutil/fault-injection-util.h
M be/src/util/error-util-test.cc
M common/thrift/generate_error_codes.py
A tests/custom_cluster/test_rpc_timeout.py
M tests/query_test/test_lifecycle.py
M tests/verifiers/metric_verifier.py
17 files changed, 397 insertions(+), 64 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/23
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 23
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

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

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

Patch Set 20:

(24 comments)

Need to look through the last few files still but here's the first set of comments.

http://gerrit.cloudera.org:8080/#/c/3343/20//COMMIT_MSG
Commit Message:

Line 12: Impala doesn't set socket send/recv timeout for backend client.
Prior to this change, ...

PS20, Line 24: new RPC call RetryRpcRecv()
This makes it sound like RetryRpcRecv() is a new RPC. I haven't looked at the change yet, but I wouldn't expect it to be. Clarify.

PS20, Line 28: call
delete (C of RPC stands for call)

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

PS20, Line 102: call
delete

PS20, Line 102: which trigger caller hits RPC timeout.
to trigger an RPC timeout on the caller side.

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

Line 162: client_map_.erase(client);
is it legal to use the 'client' iterator even though the lock was dropped in the mean time? i.e. don't we need to re-find in case the map has changed?

Line 163: *client_key = NULL;
why doesn't this method need to remove from the per_host_caches_ as well? The header comment seems to imply that happens.

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

PS20, Line 96: cache
is this "the per-host cache" that is referenced everywhere else, or something different? clarify.

PS20, Line 96: it
what is "it"? the client? or the connection? does this NULL out *client_key like ReleaseClient()?

PS20, Line 224: indicate
indicates

Is retry_is_safe an in parameter or out parameter? Clarify this.
also, is it the case that *return_is_safe is only relevant when returned status is not okay?

Line 225: /// the whole RPC call.
explain this more. is it because if the send RPC call failed, then the handler was never executed to process the RPC. On the other hand, if the send call completed, the RPC can't be restarted because the handler is already in progress. Is that right?

Line 260: if (!status.ok()) {
so this is the only path where we have *retry_is_safe == true and an error is returned, right? If so, why not set *retry_is_safe to false by default (line 240), and then only set it to true here. That would make the intent of the code clearer because then it's calling out the exceptional failure path where we can retry.

Line 265: } catch (apache::thrift::TException& e) {
why does this path not need the timeout check also?

Line 271: client_is_unrecoverable_ = false;
why is this logic needed now, whereas it wasn't needed before? or is this another bug fix that would have been beneficial even without the retry logic?

PS20, Line 301: the
whether the

but why not just call this 'last_rpc_succeeded_' to make it more self explanatory?

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

Line 115: ImpalaBackendClientCache* client_cache_;
now that we store the RuntimeState, let's get rid of this and just get the cache from the runtime state when needed (which isn't very perf critical), to make things easier to follow.

PS20, Line 153: wait
retry waiting for the response?

PS20, Line 153: call
delete "call", and would be nice to write RPC

PS20, Line 231: timeout_ms
where is this called with a non-zero timeout_ms?

Line 239: timeout_ms));
this will throw away the original status message. Maybe use AddDetail() instead on the original status.

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/service/fragment-exec-state.cc
File be/src/service/fragment-exec-state.cc:

Line 125: if (!retry_is_safe) break;
so i guess this was a bug in the old code?

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:

Line 38: FAULT_INJECTION_RPC_DELAY(RPC_EXECPLANFRAGMENT);
is there a reason we decided to put these here rather than in impala-internal-services.h handlers, which would then show we've marked up all the RPCs?

http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

PS20, Line 658: Rewrite
Add details to

PS20, Line 669: Rewrite
same

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

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#4).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

Added send/recv timeout to all rpc calls to avoid query hang. And fix
a bug that reporting thread not quiting even after query is cancelled.
For catalog client, keep default timeout to 0. ExecDdl() could take
very long time if table has many partitons, mainly waiting for HMS
API call.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/4
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.

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 <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has uploaded a new patch set (#12).

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................

IMPALA-3575: Add retry to backend connection request and rpc timeout

This patch adds a configurable timeout for all backend client
RPC calls to avoid query hang issue.

If an RPC call fails, we don't put the underlying connection back to
cache but close it. This is to make sure bad state of this connection
won't cause more RPC failure.

git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/43/3343/12
--
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 12
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout

Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.

Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 20:

(3 comments)

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

PS20, Line 224: indicate
> Right, but in the case of success, it's not really a "retry". That's just s
Done


Line 265:       } catch (apache::thrift::TException& e) {
> I'm fine with leaving this alone for now but it seems a little weird.
Let me add a Todo here.


http://gerrit.cloudera.org:8080/#/c/3343/20/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS20, Line 231: timeout_ms
> It'd be preferred to remove the timeout_ms param and this extra logic until
Done


-- 
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: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Alan Choi <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes