Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: allow disabling response stream pre-fetch#30

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
gcf-merge-on-green merged 5 commits intogoogleapis:masterfromplamut:iss-25
Jun 9, 2020

Conversation

@plamut
Copy link
Contributor

@plamutplamut commentedMay 21, 2020
edited
Loading

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 thestalled 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 throughResumableBidiRpc (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.pyindex 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 thegoogle-api-core version pin in PubSub.

PR checklist

  • Make sure to open an issue as abug/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)

@plamutplamut requested a review fromcrwilcoxMay 21, 2020 14:05
@googlebotgooglebot added the cla: yesThis human has signed the Contributor License Agreement. labelMay 21, 2020
@crwilcox
Copy link
Contributor

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
ContributorAuthor

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
ContributorAuthor

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

@busunkim96busunkim96 mentioned this pull requestJun 5, 2020
@space55
Copy link

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

@crwilcoxcrwilcox added the automergeMerge the pull request once unit tests and other checks pass. labelJun 9, 2020
@gcf-merge-on-greengcf-merge-on-greenbot merged commit74e0b0f intogoogleapis:masterJun 9, 2020
@plamutplamut deleted the iss-25 branchJune 9, 2020 21:45
@plamut
Copy link
ContributorAuthor

plamut commentedJun 9, 2020
edited
Loading

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

@space55
Copy link

Thanks! I'm excited!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@crwilcoxcrwilcoxcrwilcox approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automergeMerge the pull request once unit tests and other checks pass.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1.17.0 breaking pubsub

6 participants

@plamut@crwilcox@busunkim96@space55@arithmetic1728@googlebot

[8]ページ先頭

©2009-2025 Movatter.jp