impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
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:

(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

Mime
View raw message