impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5558/IMPALA-5576: Reopen stale client connection
Date Sun, 25 Jun 2017 21:06:10 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5558/IMPALA-5576: Reopen stale client connection

Patch Set 9:

File be/src/rpc/

PS9, Line 200:  
nit: extra space
File be/src/runtime/client-cache.h:

PS9, Line 243: ) {
not suggesting we do this now, but why do we not handle recv cxn closed errors in a similar
way as we do here, i.e. not resulting in a query failure as RPC_GENERAL_ERROR does. This question
comes from looking at the query failure cases in

PS9, Line 301: rpc send $3 done
the rpc send wasn't done is a bit awkward, can you reword?

"... rpc send completed: true/false"
File be/src/runtime/

PS9, Line 247: duplicated
nit: duplicate
File be/src/runtime/coordinator-backend-state.h:

PS9, Line 138: duplicated
nit: duplicate

PS9, Line 138: is done
can you make this more specific here? e.g. maybe "true if the final report has been received
for the fragment instance."
File be/src/runtime/

PS9, Line 227: d
nit: duplicate
File be/src/testutil/fault-injection-util.h:

PS9, Line 64:   ///
not clear this is the mod

PS9, Line 66: freq
maybe this should be every_nth_rpc or such

it'd be nice to have FAULT_INJECTION_SEND_RPC_EX and FAULT_INJECTION_RECV_RPC_EX rather than
having to pass a bool. It's fine if you don't wanna do that in this patch, but leave a TODO
at least
File tests/custom_cluster/

PS9, Line 72: execute_test_query("Called read on non-open socket"
why does this result in a query failure whereas recv_timed_out does not ?  I see in the code
where the handling is different, but not sure if there's a reason we should be failing the
query here and not in recv_timed_out? I'm not suggesting we should change this now either

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message