impl(bigtable): BulkMutator keeps pending mutations' last status
#9688
Conversation
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #9688 +/- ##
==========================================
- Coverage 93.89% 93.89% -0.01%
==========================================
Files 1496 1496
Lines 139303 139384 +81
==========================================
+ Hits 130803 130876 +73
- Misses 8500 8508 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| pending_annotations_.push_back(annotation); | ||
| pending_annotations_.push_back(Annotations{ | ||
| annotation.original_index, annotation.idempotency, | ||
| annotation.has_mutation_result, std::move(annotation.status)}); |
There was a problem hiding this comment.
What state do these moves leave annotations_[i].status in?
There was a problem hiding this comment.
So I realize now that I should have left annotation.status as a local temporary status. That is how I am using it.
There was a problem hiding this comment.
Fixed. hopefully it is a little more clear where the annotation.status comes from (pending_mutations_.push_back(...)) and when the annotation.status matters (reporting fails in OnRetryDone())
47a20da to
e0009b3
Compare
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
| * status, we return a `FailedMutation` made from `original_index` and | ||
| * `status`. The value is meaningless if `has_mutation_result` is false. | ||
| */ | ||
| Status const status; |
There was a problem hiding this comment.
The const makes the struct unassignable, which is weird, and (I'd say) isn't worth it. Should other fields be const too, for example.
There was a problem hiding this comment.
has_mutation_result can't be const. The other fields could be. I will take your suggestion and drop the const here.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Part of the work for #7479
In order to report the proper errors for bulk mutations that end on OK streams, the
BulkMutatorneeds to keep track of the last known status for a mutation it may or may not retry. There is always one copy translating fromgrpc::Status->google::cloud::Status.std::move()theStatusfrom then on.In the case where a mutation fails in a stream that also fails, I return the mutation error. My guess is that it will be more informative. But we accept either in the unit test.
Note that we retry all OK streams at the moment. This will be fixed in either 1, 2, or 4 follow up PRs. (I haven't decided)
This change is