Bulk mutator fixes #1880
Bulk mutator fixes #1880
Conversation
Whenever a mutation was not confirmed by the server (e.g. because the stream was broken every retry), it was returned as failed (correct) with -1 as original index (incorrect). This patch fixes it.
If BulkMutator never gets a response for a mutation it used to return OK, which was very confusing - it broke the invariant that every failure has a non-zero exit code, which I believe users would assume.
googlebot
added
the
cla: yes
kokoro-team
removed
the
kokoro:run
|
This is required for #1881. |
Codecov Report
@@ Coverage Diff @@
## master #1880 +/- ##
==========================================
- Coverage 92.88% 92.85% -0.03%
==========================================
Files 297 298 +1
Lines 16790 16884 +94
==========================================
+ Hits 15595 15678 +83
- Misses 1195 1206 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @coryan and @dopiera)
google/cloud/bigtable/internal/bulk_mutator.cc, line 165 at r1 (raw file):
int idx = 0; for (auto& mutation : *pending_mutations_.mutable_entries()) { auto &annotation = pending_annotations_[idx++];
I'm new to the bigtable code here, so please forgive my newbie question. But it likes there is an implicit assumption/guarantee that pending_mutations_ and pending_annotations_ are the same size. That is they are "parallel arrays" (or containers) of some objects, where the items at index i are related.
If that's the case, should we consider making this implicit assumption an explicit requirement in the code? To be specific, maybe we should consider having the BulkMutator class replace both the pending_mutations_ and pending_annotations collections with a single data member like:
struct MutationAnnotation {
google::bigtable::v2::Mutation mutation;
Annotations annotation;
};
std::vector<MutationAnnotation> pending_;I wonder if that would result in clearer code that is also easier to use and iterate. For example, here you wouldn't need to keep a separate index counter; you would only need a simple range-for loop.
Thoughts?
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @coryan and @devjgm)
google/cloud/bigtable/internal/bulk_mutator.cc, line 165 at r1 (raw file):
Previously, devjgm (Greg Miller) wrote…
I'm new to the bigtable code here, so please forgive my newbie question. But it likes there is an implicit assumption/guarantee that pending_mutations_ and pending_annotations_ are the same size. That is they are "parallel arrays" (or containers) of some objects, where the items at index
iare related.If that's the case, should we consider making this implicit assumption an explicit requirement in the code? To be specific, maybe we should consider having the BulkMutator class replace both the pending_mutations_ and pending_annotations collections with a single data member like:
struct MutationAnnotation { google::bigtable::v2::Mutation mutation; Annotations annotation; }; std::vector<MutationAnnotation> pending_;I wonder if that would result in clearer code that is also easier to use and iterate. For example, here you wouldn't need to keep a separate index counter; you would only need a simple range-for loop.
Thoughts?
Hi Greg,
I think you're right. How about I do what you're suggesting in another PR, though, not to mix the bugfix with refactoring?
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @coryan and @devjgm)
google/cloud/bigtable/internal/bulk_mutator.cc, line 165 at r1 (raw file):
Previously, dopiera (Marek Dopiera) wrote…
Hi Greg,
I think you're right. How about I do what you're suggesting in another PR, though, not to mix the bugfix with refactoring?
Actually, after I started writing the refactor, I realized that pending_mutations_ is a single proto. In order to achieve what you're suggesting, we'd have to keep the proto's entries in a vector and serialize them right before sending. I think that would actually add complexity and cost, so we shouldn't do it. Does this make sense?
I see. OK. Thanks for explaining. |
|
Did you close it on purpose? I think we need this fix. |
|
D'oh! sorry. |
coryan
removed
the
kokoro:run
| for (auto& mutation : *pending_mutations_.mutable_entries()) { | ||
| result.emplace_back( | ||
| FailedMutation(SingleRowMutation(std::move(mutation)), ok_status)); | ||
| auto &annotation = pending_annotations_[idx++]; |
There was a problem hiding this comment.
nit: Please place the & adjacent to the auto, not the variable name. per the style guide.
There was a problem hiding this comment.
I removed the annotation variable altogether.
|
LGTM, please let's wait until Kokoro has a chance to run over this. |
kokoro-team
removed
the
kokoro:run
Codecov Report
@@ Coverage Diff @@
## master #1880 +/- ##
==========================================
- Coverage 92.88% 92.85% -0.03%
==========================================
Files 297 298 +1
Lines 16790 16884 +94
==========================================
+ Hits 15595 15678 +83
- Misses 1195 1206 +11
Continue to review full report at Codecov.
|
This addresses two issues in
BulkMutator:Whenever a mutation was not confirmed by the service (e.g. because the stream was broken every retry), it was returned as failed (correct) with -1 as original index (incorrect).
In such cases OK was returned, which was very confusing - it broke the invariant that every failure has a non-zero exit code, which I believe users would assume.
This change is