impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Juan Yu (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3575: Add retry to backend connection request and rpc timeout
Date Wed, 29 Jun 2016 23:35:39 GMT
Juan Yu has posted comments on this change.

Change subject: IMPALA-3575: Add retry to backend connection request and rpc timeout
......................................................................


Patch Set 15:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/3343/15/be/src/runtime/client-cache.h
File be/src/runtime/client-cache.h:

Line 97:   void DestroyClient(ClientKey* client_key);
> needs a comment
Done


PS15, Line 197: hasRpcError_
> C++-style naming
Done


PS15, Line 239: IsTimeoutTException
> should now be called IsRecvTimeoutTException()
Done


PS15, Line 240: RPC_TIMEOUT
> should be RPC_RECV_TIMEOUT
Done


PS15, Line 246: if (strstr(e.what(),"unknown result") != NULL)
> this seems very brittle, and at least should be wrapped in an IsXXXExceptio
Done


PS15, Line 268: release
> back pressure is added by a blocking RPC, not released.
Done


PS15, Line 273: timeout
> what's the unit?
Done


PS15, Line 272: Status DoRpcTimedWait(const F& f, const Request& request, Response*
response,
              :       const G& wait_func, uint64_t timeout, RuntimeState* state, bool*
can_retry = NULL) 
> This seems like it breaks abstractions: not all RPCs happen in the context 
I agree with you. I did it this way in patch#6. but had to duplicate the wait response loop
for several rpc calls so I changed it later.


PS15, Line 282: bool no_timeout = timeout == 0;
> this can be simplified by having a deadline variable:
Done


Line 290:           if (!IsTimeoutTException(e)) {
> Just curious, which function in recv_TransmitData() throws timeout exceptio
The exception is thrown by thrift, see TSocket.cpp read()
recv_TransmitData() will call thrift api to read data from server side.


PS15, Line 309: bool hasRpcError_;
> the role of this variable is not clear. Please add a comment.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/3343
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Alan Choi <alan@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message