impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Juan Yu (Code Review)" <>
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:

File be/src/runtime/client-cache.h:

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

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

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

PS15, Line 240: RPC_TIMEOUT
> should be RPC_RECV_TIMEOUT

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

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

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

PS15, Line 272: Status DoRpcTimedWait(const F& f, const Request& request, 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:

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.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6723cfe58df6217f4a9cdd12facd320cbc24964
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Juan Yu <>
Gerrit-Reviewer: Alan Choi <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Huaisi Xu <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message