Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201606292335.u5TNZdBo000584@ip-10-146-233-104.ec2.internal>
Date: Wed, 29 Jun 2016 23:35:39 +0000
From: "Juan Yu (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, dev@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Alan Choi <alan@cloudera.com>,
        Dan Hecht <dhecht@cloudera.com>, Huaisi Xu <hxu@cloudera.com>,
        Sailesh Mukil <sailesh@cloudera.com>
Reply-To: jyu@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-CR=5D=28cdh5-trunk=29_IMPALA-3575=3A_Add_retry_to_backend_connection_request_and_rpc_timeout=0A?=
In-Reply-To: <gerrit.1465447029000.Id6723cfe58df6217f4a9cdd12facd320cbc24964@gerrit.cloudera.org>
References: <gerrit.1465447029000.Id6723cfe58df6217f4a9cdd12facd320cbc24964@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Wed, 29 Jun 2016 23:35:45 -0000

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