impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5388: Only retry RPC on lost connection in send call
Date Tue, 06 Jun 2017 06:12:18 GMT
Michael Ho has posted comments on this change.

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

Patch Set 6:

File be/src/rpc/

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;
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
File be/src/testutil/

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

Line 77:                                   "Called read on non-open socket");
> Isn't this branch supposed to be one level higher - when !is_send?
File be/src/testutil/fault-injection-util.h:

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

PS4, Line 55: inject
> injects an exception in a specified
File tests/custom_cluster/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I176975f2aa521d5be8a40de51067b1497923d09b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Alan Choi <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Juan Yu <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message