Bigtable BulkApply does not respect custom retry policies for individual mutation errors

[dbolduc]

Problem

We use the current retry policy (which can be customized by the application) to determine whether stream failures are transient.

However, we use the default retry policy to determine whether individual mutation errors are transient. Specifically, we only add mutations with default transient errors to the next batch:

google-cloud-cpp/google/cloud/bigtable/internal/bulk_mutator.cc

Lines 109 to 110 in 0117ba9

	// Failed responses are handled according to the current policies.
	if (SafeGrpcRetry::IsTransientFailure(status) &&

Implementation notes

• The annoying this is that the BulkMutatorState code path is shared by a client with Options and one without. So there will not always be a retry policy available. We likely will have to have branching in the common code path, which is a red-flag that it should not be a common code path. 🤷

• We should only check if the error is transient, with IsPermanentFailure() const. We should not be pinging OnFailure() for individual mutations. (e.g. consider a DataLimitedErrorCountPolicy(...). We want that to apply to stream failures only, not individual mutations).

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

  Lines 71 to 73 in 0117ba9

  	bool IsPermanentFailure(Status const& s) const override {
  	return impl_.IsPermanentFailure(s);
  	}

• We do not need to check IsExhausted() const. That will be done in the DataConnection's retry loop.

  google-cloud-cpp/google/cloud/bigtable/internal/data_connection_impl.cc

  Line 181 in 0117ba9

  auto status = mutator.MakeOneRequest(*stub_, *limiter_, *current);