-
Notifications
You must be signed in to change notification settings - Fork 207
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
Resolve Incompatibility with API Core >= 1.17.0 #93
Comments
I'll treat this as a bug, since Firestore works with the new API Core just fine. |
@crwilcox When running the script, how long is "eventually" in a typical case? Minutes? Hours? Was the issue consistently reproducible? For some reason the reproduction is much more difficult this time, maybe because the topic I used back then had much more messages posted or something, trying various things ATM ... Edit: OK, finally reproduced, it took less than 15 minutes in this final run. |
A few notes:
|
@crwilcox I think I've figured it out. When a
The gRPC thread calls ResumableBidiRpc._reopen() and acquires The gRPC thread then calls self._start_rpc() (== As of Is there a way to fix that Firestore bug without blocking on instance creation? Otherwise we might need to overhaul the BiDi stream reopening / locking logic, which would come with its own risks. |
@plamut and I chatted quick. The plan is to add a flag so that pubsub can elect not to start streaming from init. I have some concern that the way Pub/Sub and Firestore are currently initializing and resuming isn't compatible though and we may want to bring them into alignment with one another... |
@crwilcox Agreed - are you the right person to work with re: Firestore stuff? |
@meredithslota We discussed and decided that the It could have been the other way around, of course, but it just happened that I had more capacity and already had all the PubSub context, thus the change can be made quicker this way. P.S.: Unless you were referring to refactoring the general stream recovery logic, that's a different beast... |
Closes #25. This PR adds the ability to disable automatically pre-fetching the first item of a stream returned by `*-Stream` gRPC callables. This hook will be used in PubSub to fix the [stalled stream issue](googleapis/python-pubsub#93), while also not affecting Firestore, since the default behavior is preserved. I realize the fix is far from ideal, but it's the least ugly among the approaches I tried, e.g. somehow passing the flag through `ResumableBidiRpc` (it's a messy rabbit hole). On the PubSub side monkeypatching the generated SubscriberClient will be needed, but it's a (relatively) clean one-liner: ```patch diff --git google/cloud/pubsub_v1/gapic/subscriber_client.py google/cloud/pubsub_v1/gapic/subscriber_client.py index e98a686..1d6c058 100644 --- google/cloud/pubsub_v1/gapic/subscriber_client.py +++ google/cloud/pubsub_v1/gapic/subscriber_client.py @@ -1169,6 +1169,8 @@ class SubscriberClient(object): default_timeout=self._method_configs["StreamingPull"].timeout, client_info=self._client_info, ) + # TODO: explain this monkeypatch! + self.transport.streaming_pull._prefetch_first_result_ = False return self._inner_api_calls["streaming_pull"]( requests, retry=retry, timeout=timeout, metadata=metadata ``` If/when we merge this, we should also release it, and then we can add `!= 1.17.0` to the `google-api-core` version pin in PubSub. ### PR checklist - [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/python-api-core/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [x] Ensure the tests and linter pass - [x] Code coverage does not decrease (if any source code was changed) - [x] Appropriate docs were updated (if necessary)
Changes in 1.17.0 are causing an incompatibility with pubsub.
Related Issues:
googleapis/python-api-core#25
#74
Suspected changes that caused the behavior change:
googleapis/python-api-core@2b103b6#diff-d97e81006eaaf29e33270a85aead6aafR146-R148
Repro:
Result:
Eventually, subscription will fail and stop processing.
The failure exists in bidi on getting the next item via
__iter__
The text was updated successfully, but these errors were encountered: