Skip to content
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

feat: allow disabling response stream pre-fetch #30

Merged
merged 5 commits into from Jun 9, 2020

Conversation

plamut
Copy link
Contributor

@plamut plamut commented May 21, 2020

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, 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:

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

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@plamut plamut requested a review from crwilcox May 21, 2020 14:05
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2020
@crwilcox
Copy link
Contributor

crwilcox commented Jun 3, 2020

This seems like a reasonable approach. Before merging I would like to take a moment and make sure firestore is 'fixed' with the original change as well, just to make sure this isn't fixing the wrong thing :)

@plamut
Copy link
Contributor Author

plamut commented Jun 3, 2020

Sounds good, let me know when you have info on that. :)

@busunkim96
Copy link
Contributor

@crwilcox Anything I can do to help this move forward? I accidentally introduced a dependency conflict with a new release of google-api-python-client

@plamut
Copy link
Contributor Author

plamut commented Jun 5, 2020

+1, quite a few PubSub users are looking forward to the fix, too.

@busunkim96 busunkim96 mentioned this pull request Jun 5, 2020
@space55
Copy link

space55 commented Jun 9, 2020

Hi!

Is there any ETA for having this merged? We're currently blocked on a bugfix for this in our servers.

Thanks!

@arithmetic1728
Copy link
Contributor

+1 any updates on this bug fix? @plamut @crwilcox

@crwilcox crwilcox added the automerge Merge the pull request once unit tests and other checks pass. label Jun 9, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 74e0b0f into googleapis:master Jun 9, 2020
@plamut plamut deleted the iss-25 branch June 9, 2020 21:45
@plamut
Copy link
Contributor Author

plamut commented Jun 9, 2020

@space55 @arithmetic1728 The new PubSub release that includes its part of this fix is on the way.

@space55
Copy link

space55 commented Jun 9, 2020

Thanks! I'm excited!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.17.0 breaking pubsub
6 participants