Henry Robinson has posted comments on this change. Change subject: IMPALA-5388: Only retry RPC on lost connection in send call ......................................................................
Patch Set 9: Code-Review+1 (6 comments) Thanks - I think this is a great improvement. http://gerrit.cloudera.org:8080/#/c/7063/9/be/src/rpc/thrift-util.cc File be/src/rpc/thrift-util.cc: PS9, Line 194: DCHECK_EQ can you make this a static assert? Otherwise we might not hit this as early (e.g. until we run tests). http://gerrit.cloudera.org:8080/#/c/7063/9/be/src/runtime/client-cache.h File be/src/runtime/client-cache.h: PS9, Line 297: string this should be std::string (I think some google header rudely uses std :/). PS9, Line 298: hits nit: prefer "hit an" or "saw an" PS9, Line 299: client_ TNetworkAddressToString(address_) (match the other exception messages, not the log message that was removed). http://gerrit.cloudera.org:8080/#/c/7063/9/be/src/testutil/fault-injection-util.cc File be/src/testutil/fault-injection-util.cc: PS9, Line 69: if ((send_count.Load() / 1024) % 2 == 0) { it's unlikely in the extreme, but you could never take this branch if 1024 sends happen between lines 65 and 69. Maybe just cache the return from send_count.Add(1) and use it here? http://gerrit.cloudera.org:8080/#/c/7063/9/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: PS9, Line 27: TEST_QUERY can you comment to say that proper test coverage relies on sending TransmitData() > 1024 times? -- 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: 9 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
