[rootkiller6788]

BulkApply() was deciding whether a failed mutation deserved another attempt with the library's default retry traits, so a policy set through DataRetryPolicyOption only ever affected the stream errors. If your policy says PERMISSION_DENIED is worth retrying, or that UNAVAILABLE is not, the per-mutation handling ignored you.

What I changed:

• BulkMutatorState::OnRead() now takes the options for the request and asks the configured policy to classify each failed mutation. It only calls IsPermanentFailure(); the policy is never told about the error, because retrying a single mutation is not a new attempt of the operation and the retry loop around the RPC already does the accounting. That matches the notes in Bigtable BulkApply does not respect custom retry policies for individual mutation errors #14656.
• Both callers hand over what they already have: BulkMutator::MakeOneRequest() has the options, and AsyncBulkApplier keeps them around for its own use.
• With no policy configured we keep the old default classification, so nothing moves for the default setup. The default DataRetryPolicy delegates straight to SafeGrpcRetry, so this only changes behavior if you actually set a custom policy.

I added two tests: one where the policy makes an UNAVAILABLE mutation permanent, and one where it makes a PERMISSION_DENIED mutation retryable.

I could not build this on my machine (no gRPC/Abseil installed here), so the unit tests are doing the checking I normally would.

Fixes #14656

[gemini-code-assist]

Code Review

This pull request updates the Bigtable bulk mutator to respect the application-configured DataRetryPolicyOption when classifying mutation failures, falling back to default transient failure classification if none is configured. Unit tests were added to verify this behavior. The review feedback recommends replacing 1UL with std::size_t{1} in the assertions of the new unit tests to prevent portability issues and compilation warnings on platforms like Windows x64.

[gemini-code-assist]

Using 1UL can lead to portability issues or compilation warnings on platforms where std::size_t is not defined as unsigned long (such as Windows x64, where unsigned long is 32-bit and std::size_t is 64-bit). We should use std::size_t{1} to ensure type safety and portability across all supported platforms.

  ASSERT_EQ(std::size_t{1}, failures.size());

[gemini-code-assist]

Using 1UL can lead to portability issues or compilation warnings on platforms where std::size_t is not defined as unsigned long (such as Windows x64, where unsigned long is 32-bit and std::size_t is 64-bit). We should use std::size_t{1} to ensure type safety and portability across all supported platforms.

  ASSERT_EQ(std::size_t{1}, failures.size());

[colinmoy]

/gcbrun

[codecov]

Codecov Report

❌ Patch coverage is 97.10145% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.32%. Comparing base (272499d) to head (3e7ba07).
⚠️ Report is 75 commits behind head on main.

Files with missing lines	Patch %	Lines
...oogle/cloud/bigtable/internal/bulk_mutator_test.cc	96.72%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16572      +/-   ##
==========================================
+ Coverage   92.29%   92.32%   +0.02%     
==========================================
  Files        2257     2257              
  Lines      215637   215703      +66     
==========================================
+ Hits       199032   199147     +115     
+ Misses      16605    16556      -49     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.