Michael Ho has posted comments on this change. Change subject: IMPALA-5558/IMPALA-5576: Reopen stale client connection ......................................................................
Patch Set 10: (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? Injecting some faults (e.g. recv timeout) will cause caller of ExecQueryFInstances() to fail as it doesn't handle those cases unlike TransmitData(). So the queries end up failing early and that prevents us from exercising other RPCs. 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 fo Done 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 elsew Done 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 n Yes, in theory. Suppose the client cache is empty, it's possible to create a client although it's very unlikely. Also, in the case the connection wasn't reopened (e.g. recv() fails for unknown reason), this client is now considered broken so having the client connection in the loop will make sure it's destroyed and a new client will be checked out. The destructor of ClientConnection() will destroy client if client_is_unrecoverable_; -- 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: 10 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
