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 <[email protected]> Gerrit-Reviewer: Alan Choi <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
