Skip to content

Commit d80e09f

Browse files
committed
fix(bigtable): report the underlying error when DirectRow.commit times out
Change-Id: Iab8fba0c1d5eb2a55c723d57c40980c87c0256fb
1 parent 7e19b86 commit d80e09f

3 files changed

Lines changed: 123 additions & 14 deletions

File tree

‎packages/google-cloud-bigtable/google/cloud/bigtable/data/_helpers.py‎

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,40 @@ def _get_statuses_from_mutations_exception_group(
278278
return statuses
279279

280280

281+
def _get_status_from_operation_error(exc: Exception) -> status_pb2.Status:
282+
"""
283+
Helper function that returns a Status object for an exception raised by a
284+
single-row operation, used by the legacy synchronous shim.
285+
286+
When an operation runs out of time, :func:`_retry_exception_factory` raises a
287+
synthetic ``DeadlineExceeded`` and chains the errors actually seen on the wire
288+
as a ``RetryExceptionGroup`` cause. Reporting the synthetic wrapper would mask
289+
the real failure, so unwrap it and report the terminal error instead, matching
290+
what :func:`_get_statuses_from_mutations_exception_group` reports for the
291+
equivalent bulk operation.
292+
293+
The unwrapping is deliberately limited to that case. When retries stop for any
294+
other reason the primary exception *is* the terminal error and the group holds
295+
only the earlier attempts, so unwrapping would report a stale error. The
296+
synthetic exception is told apart by having no underlying response: errors
297+
returned by the server are built from a grpc error and carry one.
298+
299+
Args:
300+
exc: An exception raised by a single-row operation.
301+
Returns:
302+
status_pb2.Status: A Status proto object.
303+
"""
304+
cause = exc.__cause__
305+
if (
306+
isinstance(exc, core_exceptions.DeadlineExceeded)
307+
and exc.response is None
308+
and isinstance(cause, RetryExceptionGroup)
309+
and cause.exceptions
310+
):
311+
return _get_status(cause.exceptions[-1])
312+
return _get_status(exc)
313+
314+
281315
def _get_status(exc: Optional[Exception]) -> status_pb2.Status:
282316
"""
283317
Helper function that returns a Status object corresponding to the given exception.
@@ -287,12 +321,15 @@ def _get_status(exc: Optional[Exception]) -> status_pb2.Status:
287321
Returns:
288322
status_pb2.Status: A Status proto object.
289323
"""
290-
if (
291-
isinstance(exc, core_exceptions.GoogleAPICallError)
292-
and exc.grpc_status_code is not None
293-
):
294-
return status_pb2.Status( # type: ignore[unreachable]
295-
code=exc.grpc_status_code.value[0],
324+
if isinstance(exc, core_exceptions.GoogleAPICallError):
325+
return status_pb2.Status(
326+
code=(
327+
exc.grpc_status_code.value[0]
328+
if exc.grpc_status_code is not None
329+
else code_pb2.UNKNOWN
330+
),
331+
# str() on an api call error prepends its code, which is the thing
332+
# that is missing here, so report the message on its own.
296333
message=exc.message,
297334
details=exc.details,
298335
)

‎packages/google-cloud-bigtable/google/cloud/bigtable/row.py‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
from google.cloud.bigtable.data import mutations
2626
from google.cloud.bigtable.data import read_modify_write_rules as rmw_rules
27+
from google.cloud.bigtable.data._helpers import _get_status_from_operation_error
2728

2829
MAX_MUTATIONS = 100000
2930
"""The maximum number of mutations that a row can accumulate."""
@@ -451,7 +452,10 @@ def commit(self):
451452
in the row are left unchanged unless explicitly changed by a mutation.
452453
453454
After committing the accumulated mutations, resets the local
454-
mutations to an empty list.
455+
mutations to an empty list. This happens regardless of the outcome:
456+
the mutations are also discarded when the request fails, so a failed
457+
:meth:`commit` cannot be retried by calling :meth:`commit` again on
458+
the same row. Re-add the mutations to retry them.
455459
456460
For example:
457461
@@ -478,13 +482,7 @@ def commit(self):
478482
return status_pb2.Status(code=code_pb2.OK)
479483
except GoogleAPICallError as e:
480484
# If the RPC call returns an error, extract the error into a status object, if possible.
481-
return status_pb2.Status(
482-
code=e.grpc_status_code.value[0]
483-
if e.grpc_status_code is not None
484-
else code_pb2.UNKNOWN,
485-
message=e.message,
486-
details=getattr(e, "details", []),
487-
)
485+
return _get_status_from_operation_error(e)
488486
finally:
489487
self.clear()
490488

‎packages/google-cloud-bigtable/tests/unit/v2_client/test_row.py‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,80 @@ def test_direct_row_commit_with_unknown_exception():
479479
assert app_profile_id == call_args.app_profile_id[0]
480480

481481

482+
def _commit_with_operation_error(exc):
483+
"""Commit a row whose underlying ``mutate_row`` fails with ``exc``."""
484+
credentials = _make_credentials()
485+
client = _make_client(project="project-id", credentials=credentials, admin=True)
486+
table = _Table("projects/more-stuff", client=client, app_profile_id=None)
487+
row = _make_direct_row(b"row_key", table)
488+
row.set_cell("column_family_id", b"column", b"bytes-value")
489+
490+
with mock.patch.object(table._table_impl, "mutate_row") as mutate_row_mock:
491+
mutate_row_mock.side_effect = exc
492+
return row.commit()
493+
494+
495+
def test_direct_row_commit_unwraps_retry_exception_group():
496+
"""On timeout the data client raises a synthetic DeadlineExceeded chained
497+
with the errors seen on the wire. Report the terminal wire error, so that a
498+
single-row commit agrees with the equivalent bulk mutate_rows."""
499+
from google.api_core.exceptions import ServiceUnavailable
500+
from google.api_core.retry import RetryFailureReason
501+
from google.rpc import code_pb2
502+
503+
from google.cloud.bigtable.data._helpers import _retry_exception_factory
504+
505+
source_exc, _ = _retry_exception_factory(
506+
[ServiceUnavailable("first failure"), ServiceUnavailable("final failure")],
507+
RetryFailureReason.TIMEOUT,
508+
60.0,
509+
)
510+
511+
result = _commit_with_operation_error(source_exc)
512+
513+
assert result.code == code_pb2.Code.UNAVAILABLE
514+
assert result.message == "final failure"
515+
516+
517+
def test_direct_row_commit_reports_terminal_error_when_not_timed_out():
518+
"""When retries stop for any other reason the primary exception already is
519+
the terminal error and the chained group holds only earlier attempts, so it
520+
must not be unwrapped."""
521+
from google.api_core.exceptions import PermissionDenied, ServiceUnavailable
522+
from google.api_core.retry import RetryFailureReason
523+
from google.rpc import code_pb2
524+
525+
from google.cloud.bigtable.data._helpers import _retry_exception_factory
526+
527+
source_exc, _ = _retry_exception_factory(
528+
[ServiceUnavailable("earlier attempt"), PermissionDenied("terminal failure")],
529+
RetryFailureReason.NON_RETRYABLE_ERROR,
530+
60.0,
531+
)
532+
533+
result = _commit_with_operation_error(source_exc)
534+
535+
assert result.code == code_pb2.Code.PERMISSION_DENIED
536+
assert result.message == "terminal failure"
537+
538+
539+
def test_direct_row_commit_keeps_deadline_exceeded_returned_by_server():
540+
"""A DeadlineExceeded that came back from the server is the terminal error
541+
and is reported as-is, unlike the locally built one."""
542+
from google.api_core.exceptions import DeadlineExceeded, ServiceUnavailable
543+
from google.rpc import code_pb2
544+
545+
from google.cloud.bigtable.data.exceptions import RetryExceptionGroup
546+
547+
exc = DeadlineExceeded("server deadline exceeded", response=mock.Mock())
548+
exc.__cause__ = RetryExceptionGroup([ServiceUnavailable("earlier attempt")])
549+
550+
result = _commit_with_operation_error(exc)
551+
552+
assert result.code == code_pb2.Code.DEADLINE_EXCEEDED
553+
assert result.message == "server deadline exceeded"
554+
555+
482556
def test_direct_row_commit_with_invalid_argument():
483557
from google.cloud.bigtable_v2.services.bigtable import BigtableClient
484558

0 commit comments

Comments
 (0)