Lars Volker has posted comments on this change. Change subject: IMPALA-5558/IMPALA-5576: Reopen stale client connection ......................................................................
Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/7284/11/be/src/runtime/backend-client.h File be/src/runtime/backend-client.h: Line 54: // Cannot inject fault on recv() side as the callers cannot handle it. Why not? Should we leave a todo here? http://gerrit.cloudera.org:8080/#/c/7284/10/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS10, Line 348: . This is benign as cancellation is racy. Can you change this to explain what exactly the race is? I.e. "cannot be found in the backend because the fragment instance may have been cancelled by reason-xyz in the meantime". http://gerrit.cloudera.org:8080/#/c/7284/10/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: Line 139: /// Used to handle duplicate done ReportExecStatus RPC messages. mention "in ApplyExecStatusReport" to emphasize that this is not used elsewhere? http://gerrit.cloudera.org:8080/#/c/7284/11/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: Line 230: ImpalaBackendConnection client(ExecEnv::GetInstance()->impalad_client_cache(), Could this return 3 broken clients in a row? Why is it necessary to get a new client inside the loop? -- 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: 11 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
