Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706060612.v566CIpg012318@ip-10-146-233-104.ec2.internal>
Date: Tue, 6 Jun 2017 06:12:18 +0000
From: "Michael Ho (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Sailesh Mukil <sailesh@cloudera.com>,
        Juan Yu <jyu@cloudera.com>, Alan Choi <alan@cloudera.com>,
        Tim Armstrong <tarmstrong@cloudera.com>,
        Dan Hecht <dhecht@cloudera.com>
Reply-To: kwho@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5388=3A_Only_retry_RPC_on_lost_connection_in_send_call=0A?=
In-Reply-To: <gerrit.1496427703000.I176975f2aa521d5be8a40de51067b1497923d09b@gerrit.cloudera.org>
References: <gerrit.1496427703000.I176975f2aa521d5be8a40de51067b1497923d09b@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.7
archived-at: Tue, 06 Jun 2017 06:12:25 -0000

Michael Ho has posted comments on this change.

Change subject: IMPALA-5388: Only retry RPC on lost connection in send call
......................................................................


Patch Set 6:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7063/4/be/src/rpc/thrift-util.cc
File be/src/rpc/thrift-util.cc:

PS4, Line 183:  strstr(e.what(), "EAGA
> should we also retry for "send time out"? I mean the one throw by write().
See comments below.


Line 184: }
> if we upgrade thrift, won't we need to recheck these? how will we remember 
Added a DCHECK for the thrift version string.


Line 188:       "to match the current version of TSocket.cpp";
> That would also be accurate. I think both descriptions are correct.
In that sense, we should also retry on the exception below in write() as Juan mentioned above since we know we haven't completely finished the write in this case:

  while (sent < len) {
    uint32_t b = write_partial(buf + sent, len - sent);
    if (b == 0) {
      // This should only happen if the timeout set with SO_SNDTIMEO expired.
      // Raise an exception.
      throw TTransportException(TTransportException::TIMED_OUT,
                                "send timeout expired");
    }
    sent += b;
  }


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

PS4, Line 298: return strings::Substitute("Client $0 hits unexpected exception: $1 type= $2",
             :         client_, e.what(), typeid(e).name());
             :   }
             : 
> nit: prefer Substitute() over stringstream
Done


http://gerrit.cloudera.org:8080/#/c/7063/4/be/src/testutil/fault-injection-util.cc
File be/src/testutil/fault-injection-util.cc:

PS4, Line 51: if 
> remove std::
Actually needed.


PS4, Line 67:       throw TTransportException(TTransportExcept
> maybe DCHECK? Otherwise could be confusing if this silently fails to work a
Done


Line 77:                                   "Called read on non-open socket");
> Isn't this branch supposed to be one level higher - when !is_send?
Oops...Fixed.


http://gerrit.cloudera.org:8080/#/c/7063/4/be/src/testutil/fault-injection-util.h
File be/src/testutil/fault-injection-util.h:

PS4, Line 48: inject
> grammar ("injects delays" maybe?)
Done


PS4, Line 55: inject
> injects an exception in a specified
Done


http://gerrit.cloudera.org:8080/#/c/7063/4/tests/custom_cluster/test_rpc_exception.py
File tests/custom_cluster/test_rpc_exception.py:

PS4, Line 27: \
> not needed
It's actually needed in python for line continuation. Did I misunderstand your comment ?


PS4, Line 33: e
> indent python two spaces, not four
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I176975f2aa521d5be8a40de51067b1497923d09b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@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: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes