-
Notifications
You must be signed in to change notification settings - Fork 1.8k
chore(bigtable): Added an internal batch completed callback to the data client mutations batcher #18199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(bigtable): Added an internal batch completed callback to the data client mutations batcher #18199
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,15 +16,19 @@ | |
|
|
||
| import atexit | ||
| import concurrent.futures | ||
| import logging | ||
| import time | ||
| import warnings | ||
| from collections import deque | ||
| from typing import TYPE_CHECKING, Sequence, cast | ||
| from typing import TYPE_CHECKING, Any, Callable, Sequence, cast | ||
|
|
||
| from google.rpc import code_pb2, status_pb2 | ||
|
|
||
| from google.cloud.bigtable.data._cross_sync import CrossSync | ||
| from google.cloud.bigtable.data._helpers import ( | ||
| TABLE_DEFAULT, | ||
| _get_retryable_errors, | ||
| _get_statuses_from_mutations_exception_group, | ||
| _get_timeouts, | ||
| ) | ||
| from google.cloud.bigtable.data._metrics import ActiveOperationMetric, OperationType | ||
|
|
@@ -54,6 +58,7 @@ | |
|
|
||
| # used to make more readable default values | ||
| _MB_SIZE = 1024 * 1024 | ||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @CrossSync.convert_class(sync_name="_FlowControl", add_mapping_for_name="_FlowControl") | ||
|
|
@@ -294,6 +299,9 @@ def __init__( | |
| self._newest_exceptions: deque[Exception] = deque( | ||
| maxlen=self._exception_list_limit | ||
| ) | ||
| self._user_batch_completed_callback: ( | ||
| Callable[[list[status_pb2.Status]], Any] | None | ||
| ) = None | ||
| # clean up on program exit | ||
| atexit.register(self._on_exit) | ||
|
|
||
|
|
@@ -410,6 +418,7 @@ async def _execute_mutate_rows( | |
| list of FailedMutationEntryError objects for mutations that failed. | ||
| FailedMutationEntryError objects will not contain index information | ||
| """ | ||
| statuses = [status_pb2.Status(code=code_pb2.UNKNOWN) for _ in range(len(batch))] | ||
| try: | ||
| operation = CrossSync._MutateRowsOperation( | ||
| self._target.client._gapic_client, | ||
|
|
@@ -422,13 +431,26 @@ async def _execute_mutate_rows( | |
| ) | ||
| await operation.start() | ||
| except MutationsExceptionGroup as e: | ||
| statuses = _get_statuses_from_mutations_exception_group(e, len(batch)) | ||
|
|
||
| # strip index information from exceptions, since it is not useful in a batch context | ||
| for subexc in e.exceptions: | ||
| subexc.index = None | ||
| return list(e.exceptions) | ||
| else: | ||
| statuses = [status_pb2.Status(code=code_pb2.OK) for _ in range(len(batch))] | ||
| finally: | ||
| # mark batch as complete in flow control | ||
| await self._flow_control.remove_from_flow(batch) | ||
|
|
||
| # Call batch done callback with list of statuses. | ||
| if self._user_batch_completed_callback: | ||
| try: | ||
| self._user_batch_completed_callback(statuses) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need await self._user_batch_completed_callback ? or this can only get passed in from legacy batcher.py so it's guaranteed to be a sync method? Even so it might still be better to add await in case we somehow exposes this to user later, or maybe add a documentation saying this can only be a sync method and insepct that it doesn't return a coroutine?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah the callback is sync-only, so no need to await. This was only added so we can be compatible with the old batcher implementation, which was also sync-only. This is an internal attribute, that is never directly exposed to users (except through the shim) I'd say the type annotations on _user_batch_completed_callback should be sufficient documentation here. As long as the type is obeyed and a callable is passed, there should be no issues here. But I can look into making this more async-friendly if you'd prefer that |
||
| except Exception as exc: | ||
| _LOGGER.warning( | ||
| f"Exception raised in user batch completion callback: {exc}" | ||
| ) | ||
| return [] | ||
|
|
||
| def _add_exceptions(self, excs: list[Exception]): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| TYPE_CHECKING, | ||
| Callable, | ||
| List, | ||
| Optional, | ||
| Sequence, | ||
| Tuple, | ||
| Union, | ||
|
|
@@ -32,8 +33,12 @@ | |
| from google.api_core import exceptions as core_exceptions | ||
| from google.api_core import retry as retries | ||
| from google.api_core.retry import RetryFailureReason, exponential_sleep_generator | ||
| from google.rpc import code_pb2, status_pb2 | ||
|
|
||
| from google.cloud.bigtable.data.exceptions import RetryExceptionGroup | ||
| from google.cloud.bigtable.data.exceptions import ( | ||
| MutationsExceptionGroup, | ||
| RetryExceptionGroup, | ||
| ) | ||
| from google.cloud.bigtable.data.read_rows_query import ReadRowsQuery | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -238,6 +243,66 @@ def _align_timeouts(operation: float, attempt: float | None) -> tuple[float, flo | |
| return operation, final_attempt | ||
|
|
||
|
|
||
| def _get_statuses_from_mutations_exception_group( | ||
| exc_group: MutationsExceptionGroup, batch_size: int | ||
| ) -> list[status_pb2.Status]: | ||
| """ | ||
| Helper function that populates a list of Status objects with exception information from | ||
| the exception group. | ||
|
|
||
| Args: | ||
| exc_group: The exception group from a mutate rows operation | ||
| batch_size: How many RowMutationGroups were provided to the batch | ||
| Returns: | ||
| list[status_pb2.Status]: A list of Status proto objects | ||
| """ | ||
| # We exception handle as follows: | ||
| # | ||
| # 1. Each exception in the error group is a FailedMutationEntryError, and its | ||
| # cause is either a singular exception or a RetryExceptionGroup consisting of | ||
| # multiple exceptions. | ||
| # | ||
| # 2. In the case of a singular exception, if the error does not have a gRPC status | ||
| # code, we return a status code of UNKNOWN. | ||
| # | ||
| # 3. In the case of a RetryExceptionGroup, we use terminal exception in the exception | ||
| # group and process that. | ||
| statuses = [status_pb2.Status(code=code_pb2.OK) for _ in range(batch_size)] | ||
| for error in exc_group.exceptions: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what handles the rpc level error?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. _MutateRowsOperation converts grpc-level errors into FaledMutationEntryErrors for each entry |
||
| if isinstance(error.index, int) and 0 <= error.index < len(statuses): | ||
| cause = error.__cause__ | ||
| if isinstance(cause, RetryExceptionGroup): | ||
| statuses[error.index] = _get_status(cause.exceptions[-1]) | ||
| else: | ||
| statuses[error.index] = _get_status(cause) | ||
| return statuses | ||
|
|
||
|
|
||
| def _get_status(exc: Optional[Exception]) -> status_pb2.Status: | ||
| """ | ||
| Helper function that returns a Status object corresponding to the given exception. | ||
|
|
||
| Args: | ||
| exc: An exception to be converted into a Status. | ||
| Returns: | ||
| status_pb2.Status: A Status proto object. | ||
| """ | ||
| if ( | ||
| isinstance(exc, core_exceptions.GoogleAPICallError) | ||
| and exc.grpc_status_code is not None | ||
| ): | ||
| return status_pb2.Status( # type: ignore[unreachable] | ||
| code=exc.grpc_status_code.value[0], | ||
| message=exc.message, | ||
| details=exc.details, | ||
| ) | ||
|
|
||
| return status_pb2.Status( | ||
| code=code_pb2.UNKNOWN, | ||
| message=str(exc) if exc else "An unknown error has occurred", | ||
| ) | ||
|
|
||
|
|
||
| def _validate_timeouts( | ||
| operation_timeout: float, attempt_timeout: float | None, allow_none: bool = False | ||
| ): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.