impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5558/IMPALA-5576: Reopen stale client connection
Date Mon, 26 Jun 2017 04:29:31 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message