fix(bigtable): data client should acknowledge all mutations in batch - #18124
fix(bigtable): data client should acknowledge all mutations in batch#18124mutianf wants to merge 1 commit into
Conversation
…dged Change-Id: I9a166818c5829b446553fc1427d1f1d1ec078d0d
There was a problem hiding this comment.
Code Review
This pull request introduces a response completeness check to both the async and sync implementations of _mutate_rows.py, ensuring that any mutation entries not acknowledged by the server are explicitly failed with a ClientError rather than being silently treated as successful. While the added test coverage is thorough, the reviewer correctly identified a critical bug in both implementations: when mutations are retried across multiple attempts, obsolete acknowledgments and errors from previous attempts are not cleared, which prevents the completeness check from correctly identifying silently dropped entries in subsequent attempts. To resolve this, the state of retried entries should be cleared at the start of each attempt.
| if len(self._acknowledged_indices) != len(self.mutations): | ||
| for idx in range(len(self.mutations)): | ||
| if idx not in self._acknowledged_indices and ( | ||
| idx not in self.errors | ||
| ): |
There was a problem hiding this comment.
Bug: Unacknowledged entries in retried attempts will not be correctly failed with ClientError
There is a subtle bug here when mutations are retried across multiple attempts:
-
Scenario:
- A mutation entry at index
ifails with a retryable error (e.g.,UNAVAILABLE) in Attempt 1. - This adds
itoself._acknowledged_indicesand the retryable error toself.errors[i]. - Since it's retryable, it is retried in Attempt 2.
- In Attempt 2, the stream finishes successfully but silently drops/ignores index
i(i.e., no response entry is returned for it, and no stream-level exception is raised).
- A mutation entry at index
-
The Issue:
- In the
finallyblock,len(self._acknowledged_indices) != len(self.mutations)is checked. Sinceiwas acknowledged in Attempt 1, it is already inself._acknowledged_indices. If all other entries were acknowledged, this check will evaluate toFalseand the completeness check won't run. - Even if it runs,
idx not in self._acknowledged_indiceswill beFalseandidx not in self.errorswill beFalse(due to the error from Attempt 1). - Thus, the entry will not be marked with
ClientError. Instead, the operation will fail with the obsolete retryable error from Attempt 1, which might cause the client to incorrectly retry the entire batch.
- In the
-
Recommended Solution:
At the start of each attempt (e.g., at the beginning of_run_attempt), we should clear the state of the entries being retried so that we only consider acknowledgments and errors from the current attempt:self._acknowledged_indices.difference_update(self.remaining_indices) for idx in self.remaining_indices: self.errors.pop(idx, None)
| if len(self._acknowledged_indices) != len(self.mutations): | ||
| for idx in range(len(self.mutations)): | ||
| if ( | ||
| idx not in self._acknowledged_indices | ||
| and idx not in self.errors | ||
| ): |
There was a problem hiding this comment.
Bug: Unacknowledged entries in retried attempts will not be correctly failed with ClientError
This is the same issue as identified in the async version (_async/_mutate_rows.py). When mutations are retried across multiple attempts, obsolete acknowledgments and errors from previous attempts are not cleared, preventing the completeness check from correctly identifying and failing silently dropped entries in the final attempt.
Recommended Solution:
At the start of each attempt (e.g., at the beginning of _run_attempt), clear the state of the entries being retried:
self._acknowledged_indices.difference_update(self.remaining_indices)
for idx in self.remaining_indices:
self.errors.pop(idx, None)|
Did your original approach not work? I thought that seemed like a good solution. Gemini pointed out some issues with the new approach. It could be difficult to keep a global |
fail V3 mutate_rows entries the server never acknowledged