-
Notifications
You must be signed in to change notification settings - Fork 87
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
fix: ensure exception is available when BackgroundConsumer open stream fails #357
Conversation
7f05e51
to
767b215
Compare
767b215
to
1a64fff
Compare
dddb4d3
to
97c2363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Let's block this until we figure out google-cloud-bigquery-storage
though. googleapis/python-bigquery-storage#414
google/api_core/bidi.py
Outdated
try: | ||
call = self._start_rpc(iter(request_generator), metadata=self._rpc_metadata) | ||
except exceptions.GoogleAPICallError as exc: | ||
self._on_call_done(exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on_call_done expects a future (based on the name of its argument). How does it work with an exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception will be raised in the call back add_done_callback
and must be handled by the user. This is based on the comment here:
python-api-core/google/api_core/bidi.py
Lines 608 to 610 in 8f73d2e
Note that error handling *must* be done by using the provided | |
``bidi_rpc``'s ``add_done_callback``. This helper will automatically exit | |
whenever the RPC itself exits and will not provide any error details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change that I proposed is exactly the same behaviour as the _on_call_done
method of ResumableBidiRpc
python-api-core/google/api_core/bidi.py
Lines 441 to 443 in 6251eab
# Unlike the base class, we only execute the callbacks on a terminal | |
# error, not for errors that we can recover from. Note that grpc's | |
# "future" here is also a grpc.RpcError. |
From grpc/grpc#10885 (comment), grpc.RpcError
is also grpc.Call
.
I added this note in c7f63bd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted c7f63bd because GoogleAPICallError
is not grpc.Call
or grpc.RpcError
. The original grpc.RpcError
is available on the response
property of GoogleAPICallError
python-api-core/google/api_core/grpc_helpers.py
Lines 174 to 182 in 6251eab
def wrap_errors(callable_): | |
"""Wrap a gRPC callable and map :class:`grpc.RpcErrors` to friendly error | |
classes. | |
Errors raised by the gRPC callable are mapped to the appropriate | |
:class:`google.api_core.exceptions.GoogleAPICallError` subclasses. | |
The original `grpc.RpcError` (which is usually also a `grpc.Call`) is | |
available from the ``response`` property on the mapped exception. This | |
is useful for extracting metadata from the original error. |
The behaviour proposed in this PR is the same as ResumableBidiRpc
and we typically raise GoogleAPICallError
instead of grpc.RpcError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the code in 753a591 to raise grpc.RpcError
instead of GoogleAPICallError
since grpc.RpcError
is also grpc.Call
and it is a better fit
@loferris, please could you review/approve? |
@rosiezou You might be interested in this PR. I forget the full context, but I know it's something I wanted for better debugging of the Python BQ Storage Write API. |
@parthea any idea when this will be merged? |
@vchudnov-g Please could you take a look? |
@@ -276,7 +276,11 @@ def open(self): | |||
request_generator = _RequestQueueGenerator( | |||
self._request_queue, initial_request=self._initial_request | |||
) | |||
call = self._start_rpc(iter(request_generator), metadata=self._rpc_metadata) | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In examining this file, it seems to me that _RequestQueueGenerator._is_active
(lines 91 et seq) should be def
d as return self.call is not None and self.call.is_active()
. The way it's currently written it will return True
when self.call == None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e120a0c
google/api_core/bidi.py
Outdated
try: | ||
call = self._start_rpc(iter(request_generator), metadata=self._rpc_metadata) | ||
except exceptions.GoogleAPICallError as exc: | ||
self._on_call_done(exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vchudnov-g Please could you take another look? |
Fixes #268🦕
This PR updates the
bidi.py
code to reflect what is stated in the comments.The comment for
add_done_callback
statesThis occurs when the RPC errors or is successfully terminated.
however the done callback was not being called when an error occurs.python-api-core/google/api_core/bidi.py
Lines 255 to 264 in 2477ab9
python-api-core/google/api_core/bidi.py
Lines 267 to 269 in 2477ab9