impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout
Date Thu, 07 Jul 2016 23:58:10 GMT
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 <jyu@cloudera.com>
Gerrit-Reviewer: Alan Choi <alan@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message