impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
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:

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()?
File be/src/runtime/

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:  

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.
File be/src/testutil/fault-injection-util.h:

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

PS21, Line 39: call

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?
File tests/custom_cluster/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <>
Gerrit-Reviewer: Alan Choi <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Huaisi Xu <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message