Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12672 )
Change subject: IMPALA-8143: Enhance DoRpcWithRetry(). ...................................................................... Patch Set 5: (3 comments) Thanks Michael. Please see the upcoming Patch Set 8. http://gerrit.cloudera.org:8080/#/c/12672/5/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/12672/5/be/src/rpc/rpc-mgr-test.cc@333 PS5, Line 333: // Call DoRpcWithRetry with no backoff on our busy service. : const int retries = 3; : impala::Status rpc_status = RpcMgr::DoRpcWithRetry(proxy, &PingServiceProxy::Ping, : request, &response, query_ctx, "ping failed", retries, MonoDelta::FromSeconds(100)); : EXPECT_ERROR(rpc_status, TErrorCode::GENERAL); : EXPECT_STR_CONTAINS(rpc_status.msg().msg(), : "dropped due to backpressure. The service queue contains 1 items out of a maximum " : "of 1; memory consumption is "); : ASSERT_EQ(overflow_metric->GetValue(), retries); : : // Call DoRpcWithRetry, but this time we do backoff on a busy service, and this waiting : // allows the outstanding aysnc calls to complete, so that then this call will succeed. : impala::Status rpc_status_backoff = : RpcMgr::DoRpcWithRetry(proxy, &PingServiceProxy::Ping, request, &response, : query_ctx, "ping failed", 10, MonoDelta::FromSeconds(100), 2); : ASSERT_OK(rpc_status_backoff); : ASSERT_GT(overflow_metric->GetValue(), retries); : : // Check that async calls did complete. : ASSERT_FALSE(async1_done.pending()); : ASSERT_FALSE(async2_done.pending()); > How about the following idea: I did think about this. So the nice thing about your suggestion (thanks) is that we can set up the full queue. Now we want to have a call to DoRpcWithRetry that will block (because queue is full) and then succeed (after retry). So concurrently with the call to DoRpcWithRetry we need to unblock the queue. We could start the call to DoRpcWithRetry in a thread, and then sleep for a while, and then unblock the queue. That would work but is functionally equivalent to what we have now in that it depends on a sleep call. So I understand the desire for a completely deterministic test but I still don't see a simple way to achieve that. I am happy to listen to more suggestions, or explanations of what I am not understanding. http://gerrit.cloudera.org:8080/#/c/12672/7/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/12672/7/be/src/rpc/rpc-mgr.h@186 PS7, Line 186: og' fun > timeout_ms Done http://gerrit.cloudera.org:8080/#/c/12672/5/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/12672/5/be/src/rpc/rpc-mgr.h@178 PS5, Line 178: // A no-op method that is used as part of overloading DoRpcWithRetry(). : static void logging_noop(){}; : : // The type of the log method passed to DoRpcWithRetry(). : typedef boost::function<void()> RpcLogFn; > They still seem to be in PS7 Done -- 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: 5 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Tue, 02 Apr 2019 18:17:46 +0000 Gerrit-HasComments: Yes
