Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12672 )
Change subject: IMPALA-8143: Enhance DoRpcWithRetry(). ...................................................................... Patch Set 10: (12 comments) Thanks for the review. See patch set 10. http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.h File be/src/rpc/rpc-mgr-test.h: http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.h@134 PS9, Line 134: ultipleServicesTest(Rp > Coding style suggests this should be "GetCurrentQueueSize()". With that sai Done http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.h@295 PS9, Line 295: ingServiceProxy { : public: > Instead of IOError, can we simulate the server busy error here ? Please see I'm using your idea but keeping this class as-is for simplicity http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.cc@269 PS9, Line 269: uery_c > typo Done http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.cc@285 PS9, Line 285: : // Test injection of failures with DebugAction. : query_ctx.client_request.query_options.__set_debug_action("DoRpcWithRetry:FAIL"); : PingRequestPB request3; : PingResponsePB response3; : Status inject_status = RpcMgr::DoRpcWithRetry(busy_proxy, &BusyPingServiceProxy::Ping, : request3, &response3, query_ctx, "ping failed", num_retries, timeout_ms, 0, : "DoRpcWithRetry"); : ASSERT_FALSE(inject_status.ok()); : EXPECT_ERROR(inject_status, TErrorCode::INTERNAL_ERROR); : ASSERT_EQ("Debug Action: DoRpcWithRetry:FAIL", inject_status.msg().msg()); : } : : } // namespace impala : : int main(int argc, char** argv) { : ::testing::InitGoogleTest(&argc, argv); : impala::InitCommonRuntime(argc, argv, true, impala::TestInfo::BE_TEST); : impala::InitFeSupport(); : : // Fill in the path of the current binary for use by the tests. : CURRENT_EXECUTABLE_PATH = argv[0]; : return RUN_ALL_TESTS(); : } : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : > Stepping back a bit, it's unclear the exact purposes of these tests are. Done http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.cc@376 PS9, Line 376: : : > Not entirely sure this test is needed as the EE test should exercise it alr This test is small and ensures that the DebugAction code is working correctly. The EE test seems fragile to me in that it does not check if an error actually was injected. http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr-test.cc@380 PS9, Line 380: : : : : : : : : : : : > This test seems to be sufficient for the coverage we need if we make the fo Done, though note that there is some slightly tricky code to make the service appear busy - we can't just return a ServerBusy status, instead we have to inject an error into the Rpc Controller. http://gerrit.cloudera.org:8080/#/c/12672/10/be/src/rpc/rpc-mgr.inline.h File be/src/rpc/rpc-mgr.inline.h: http://gerrit.cloudera.org:8080/#/c/12672/10/be/src/rpc/rpc-mgr.inline.h@67 PS10, Line 67: rpc_status = DebugAction(query_ctx.client_request.query_options, debug_action); I think this is still needed by existing tests. I'll leave Thomas to sort out the future solution. http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr.inline.h File be/src/rpc/rpc-mgr.inline.h: http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/rpc/rpc-mgr.inline.h@75 PS9, Line 75: // If the call succeeded, or the server is not busy, then return result to caller. : if (rpc_status.ok() || !RpcMgr::IsServerTooBusy(rpc_controller)) { : return rpc_status; : } : > if (rpc_status.ok() || !RpcMgr::IsServerTooBusy(rpc_controller)) { Done http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/runtime/coordinator-backend-state.cc@465 PS9, Line 465: : Status rpc_status = > nit: May make for better documentation if we have these couple of parameter much nicer, thanks http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/service/client-request-state.cc@684 PS9, Line 684: : Status shutdown_status > nit: May make for better documentation if we have these couple of parameter Thanks http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/testutil/fault-injection-util.h File be/src/testutil/fault-injection-util.h: http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/testutil/fault-injection-util.h@37 PS9, Line 37: RPC_TRANSMITDATA, > This can also be removed. Seems to be used in data-stream-service.cc ? http://gerrit.cloudera.org:8080/#/c/12672/9/be/src/testutil/fault-injection-util.h@40 PS9, Line 40: // If you add a new RpcCallType then bump the "--fault_injection_rpc_type=X" parameter : // in the test test_random_rpc_timeout() in test_rpc_timeout.py. > Looks like we forgot to update the tests when adding fault injection for re Thanks -- To view, visit http://gerrit.cloudera.org:8080/12672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9693151c35e02235665b3c285a48c585973d390 Gerrit-Change-Number: 12672 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Fri, 19 Apr 2019 16:19:28 +0000 Gerrit-HasComments: Yes
