RPC timeouts and retry loops across services

[coryan]

The google::cloud::bigtable::RPCRetryPolicy classes have special member functions to setup a timeout:

google-cloud-cpp/google/cloud/bigtable/rpc_retry_policy.h

Lines 98 to 101 in 81adbda

	/**
	* Update the ClientContext for the next call.
	*/
	virtual void Setup(grpc::ClientContext& context) const = 0;

google-cloud-cpp/google/cloud/bigtable/rpc_retry_policy.cc

Lines 53 to 57 in 81adbda

	void LimitedTimeRetryPolicy::Setup(grpc::ClientContext& context) const {
	if (context.deadline() >= impl_.deadline()) {
	context.set_deadline(impl_.deadline());
	}
	}

This is used in the synchronous retry loop, e.g.:

google-cloud-cpp/google/cloud/bigtable/table.cc

Lines 90 to 95 in 81adbda

	while (true) {
	grpc::ClientContext client_context;
	rpc_policy->Setup(client_context);
	backoff_policy->Setup(client_context);
	metadata_update_policy_.Setup(client_context);
	status = client_->MutateRow(&client_context, request, &response);

But not in the asynchronous retry loop:

google-cloud-cpp/google/cloud/internal/async_retry_unary_rpc.h

Lines 145 to 149 in 81adbda

	static void StartIteration(std::shared_ptr<RetryAsyncUnaryRpc> self,
	CompletionQueue cq) {
	auto context = absl::make_unique<grpc::ClientContext>();
	cq.MakeUnaryRpc(self->async_call_, self->request_, std::move(context))

That asynchronous retry loop is used in spanner too, and the RetryPolicy for spanner does not have a Setup() member function. Furthermore, the same retry policy class is used in storage, where we would't want to introduce it (because that would create an unwanted dependency on gRPC).

We need a cleaner way to setup per-RPC timeouts across all services, with good support for REST, and we need to use that on the asynchronous and synchronous loops.

[devjgm]

Carlos and I just chatted about this a bit offline. I'm going to try to summarize what I think we want/need here, and this will help expose my own misunderstandings.

• We need to make sure our retry policies all properly set timeouts for both sync and async APIs. This should work for all services, whether using grpc or REST. As described in the comment above, Bigtable uses its own RPCRetryPolicy (not the common one), and Bigtable's policy does set timeouts for sync RPC, but not async. Ideally, this would be handled by the common retry policy and Bigtable would not need to provide its own.

• It's difficult to unit test with any retry policy because they sleep. It would be nice to provide a hook where tests can mock/fake out the sleep part so that unit tests will run faster and be less flaky. This should work for both sync and async retries.

• It may not be possible to make the above changes without breaking the retry policy API, which is exposed publicly to users (spanner example). While we want to avoid API breakages, it may be necessary in this case.

• A change to how retries work and the design of our retries needs a design doc.

How urgent is this? It's not work-on-the-weekends urgent, but it's pretty important. We currently have shipping products that don't have proper timeouts. We're about to ship Pub/Sub, which will have the same issues (because it's using the common retry policies), and it would be nice to not break Pub/Sub's API right after it GAs (let's try to do that before). So I think the urgency is that someone should own this bug and it should probably be their top priority until its finished.

[coryan]

nit: the retry policies do not sleep today, the retry loop does, and the sleep time comes from the backoff policy. Your bigger point about this making retry loops hard to test stands.

[devjgm]

@devbww any update on this?

[devbww]

any update on this?

No. Indeed, I am only starting to work on it directly today.

[coryan]

Turns out #5959 might be directly related to this.

[coryan]

FWIW, after #6014 the deadline is being set for asynchronous operations in the Cloud Bigtable client library, and the retry loops can handle any retry policy that has a Setup(grpc::ClientContext&) member function.

Note that the general problem of how to rationalize our retry and backoff policies remains open.

[devbww]

(internal only) Brainstorm doc

We need to make sure our retry policies all properly set timeouts for both sync and async APIs.

One of the ideas in the above brain-storming document is not to use the retry policy for RPC deadlines. The amount of time you're willing to wait to start an RPC (retry time limit) is distinct from the amount of time you're happy to let it run (deadline).

Rather, a functor option can be added where the user can set the grpc::ClientContext (deadline + ...) directly.