Michael Ho has posted comments on this change. Change subject: IMPALA-5558/IMPALA-5576: Reopen stale client connection ......................................................................
Patch Set 9: (15 comments) http://gerrit.cloudera.org:8080/#/c/7284/9//COMMIT_MSG Commit Message: PS9, Line 18: to appear succeed > nit: typo, to appear to succeed Missed that before pushing. Will update before merging. http://gerrit.cloudera.org:8080/#/c/7284/8/be/src/rpc/thrift-util.cc File be/src/rpc/thrift-util.cc: PS8, Line 198: TTransportException::END_OF_FILE && : strstr(e.what(), "No more data to read. > Can you put this in a comment as well? Done http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/rpc/thrift-util.cc File be/src/rpc/thrift-util.cc: PS9, Line 200: > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/runtime/client-cache.h 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 err Yes, this may be worth some re-thinking. FWIW, most callers cannot handle recv side failure any way. I believe only TrasmitData() / ReportExecStatusAux() and Cancel() will handle it. PS9, Line 301: rpc send $3 done > nit Done http://gerrit.cloudera.org:8080/#/c/7284/8/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS8, Line 362: true; > I was also wondering if this return value isn't very useful. What if we ins The status and the backtrace is most likely logged when it's constructed initially deep down the call stack. I will keep the return value as-is for now to avoid more unexpected complication. http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS9, Line 247: duplicated > nit: duplicate Done PS9, Line 248: if (instance_exec_status.done && instance_stats->done_) continue; > Is there any reason we want to process a not done status from a fragment th Good point. We should simply ignore it. The current check is to make sure we don't subtract num_remaining_instances_ more than once for a given fragment instance. http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: PS9, Line 138: duplicated > nit: duplicate Done PS9, Line 138: is done > can you make this more specific here? e.g. maybe "true if the final report Done http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS9, Line 227: d > nit: duplicate Done http://gerrit.cloudera.org:8080/#/c/7284/9/be/src/testutil/fault-injection-util.h File be/src/testutil/fault-injection-util.h: PS9, Line 64: /// > not clear this is the mod Rephrased the comment. PS9, Line 66: freq > maybe this should be every_nth_rpc or such Rephrased the comment. PS9, Line 75: FAULT_INJECTION_RPC_EXCEPTION > it'd be nice to have FAULT_INJECTION_SEND_RPC_EX and FAULT_INJECTION_RECV_R Done http://gerrit.cloudera.org:8080/#/c/7284/9/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: 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 ? Most likely, in this case, TSocket was closed (potentially due to programming error in our part). In the case of timeout, the socket is still opened but it just hits the timeout we specify when waiting for data to show up. -- To view, visit http://gerrit.cloudera.org:8080/7284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
