feat: Added rst_stream exception handling for ReadRows.#1298
feat: Added rst_stream exception handling for ReadRows.#1298gkevinzheng merged 4 commits intov3_stagingfrom
Conversation
product-auto-label
bot
added
the
api: bigtable
Summary of ChangesHello @gkevinzheng, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the resilience of Bigtable Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces custom exception handling for ReadRows to treat certain INTERNAL errors related to rst_stream as UNAVAILABLE errors, making them retryable. The implementation adds a new retry predicate _read_rows_predicate_with_exceptions and updates the _ReadRowsOperation to use it. The changes are well-tested, with new unit tests for the predicate and updates to existing tests to accommodate the new logic.
My review includes a few suggestions for improvement in google/cloud/bigtable/data/_helpers.py: fixing a typo in a comment, correcting a docstring, and making the error message check more robust by using a substring search. Overall, the changes look good and align with the goal described in the pull request.
| _DEFAULT_BIGTABLE_EMULATOR_CLIENT = "google-cloud-bigtable-emulator" | ||
|
|
||
| # Internal error messages that can be retried during ReadRows. Internal error messages with this error | ||
| # text should be streated as Unavailable error messages with the same error text, and will therefore be |
There was a problem hiding this comment.
There's a typo in the comment. streated should be treated.
| # text should be streated as Unavailable error messages with the same error text, and will therefore be | |
| # text should be treated as Unavailable error messages with the same error text, and will therefore be |
| ServiceUnavailable errors and will retry them if the Unavailable exception is retryable. | ||
|
|
||
| Args: | ||
| retryable_exceptions: tuple of Exception types to be retried during operation |
There was a problem hiding this comment.
The argument name in the docstring (retryable_exceptions) does not match the function signature (exception_types). Please correct it for consistency.
| retryable_exceptions: tuple of Exception types to be retried during operation | |
| exception_types: tuple of Exception types to be retried during operation |
| is_exception_type = retries.if_exception_type(*exception_types) | ||
|
|
||
| def predicate(exception: Exception) -> bool: | ||
| return (isinstance(exception, core_exceptions.InternalServerError) and exception.message.lower() in _RETRYABLE_INTERNAL_ERROR_MESSAGES) or is_exception_type(exception) |
There was a problem hiding this comment.
The current implementation checks if the entire error message is one of the strings in _RETRYABLE_INTERNAL_ERROR_MESSAGES. This might be too strict, as the actual error message could contain additional details. Using a substring check would be more robust to handle variations in the error message. For example, an error message like 'rst_stream with code 7' would not be caught by the current implementation.
| return (isinstance(exception, core_exceptions.InternalServerError) and exception.message.lower() in _RETRYABLE_INTERNAL_ERROR_MESSAGES) or is_exception_type(exception) | |
| return (isinstance(exception, core_exceptions.InternalServerError) and any(m in exception.message.lower() for m in _RETRYABLE_INTERNAL_ERROR_MESSAGES)) or is_exception_type(exception) |
| return source_exc, cause_exc | ||
|
|
||
|
|
||
| def _read_rows_predicate_with_exceptions(*exception_types: type[Exception]) -> Callable[[Exception], bool]: |
There was a problem hiding this comment.
can we call this something more specific, since it's not really tied to read_rows? Maybe _rst_stream_aware_predicate?
| if core_exceptions.ServiceUnavailable in exception_types and core_exceptions.InternalServerError not in exception_types: | ||
| return predicate | ||
|
|
||
| return is_exception_type |
There was a problem hiding this comment.
I find the logic a bit hard to follow, with the multiple predicate functions being passed around. Can we do something like this, to put all the cases in one place?
def _read_rows_predicate_with_exceptions(*exception_types: type[Exception]) -> Callable[[Exception], bool]:
def predicate(exception: Exception) -> bool:
if isinstance(exception, exception_types):
# handle standard retryable errors
return True
elif core_exceptions.ServiceUnavailable in exception_types and isinstance(exception, InternalServerError) and any(m in exception.message.lower() for m in _RETRYABLE_INTERNAL_ERROR_MESSAGES)):
# special case: treat InternalServerError with specific message as ServiceUnavailable
return True
else:
return False
return predicate
or even this?
def _read_rows_predicate_with_exceptions(*exception_types: type[Exception]) -> Callable[[Exception], bool]:
# predicate to check for retryable error types
if_exception_type = retries.if_exception_type(*exception_types)
# special case: treat InternalServerError with specific message as ServiceUnavailable
rst_check = lambda e: core_exceptions.ServiceUnavailable in exception_types and isinstance(e, InternalServerError) and any(m in e.message.lower() for m in _RETRYABLE_INTERNAL_ERROR_MESSAGES))
return lambda e: if_exception_type(e) or rst_check(e)
3.9 has been removed from the kokoro base image, so we should remove the tests from our configs 3.9 is still tested in GitHub Actions
2 participants
According to go/rst_stream,
INTERNALerrors with error messages related to anrst_streamerror should be interpreted asUNAVAILABLEerrors instead of internal errors. This PR creates a custom retry predicate to allow retrying ofINTERNALerrors with rst_stream specific error messages if theServiceUnavailableexception is allowed to be retried.