FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: PubSub incompatibility with api-core 1.17.0+ by plamut · Pull Request #103 · googleapis/python-pubsub · GitHub

This repository was archived by the owner on Mar 9, 2026. It is now read-only.
/ python-pubsub Public archive

fix: PubSub incompatibility with api-core 1.17.0+ - #103

Merged
plamut merged 10 commits into
googleapis:masterfrom
plamut:iss-93
Jun 9, 2020
ConversationCommits (10)ChecksFiles changed
Merged

fix: PubSub incompatibility with api-core 1.17.0+#103
plamut merged 10 commits into
googleapis:masterfrom
plamut:iss-93

Conversation

plamut commented May 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #93.

This PR fixes the bug introduced in api-core 1.17.0 that breaks streaming pull recovery on recoverable errors.

The fix depends on the related pull request in API core that allows disabling the automatic pre-fetch of the first stream result.

Probably best to not merge until a new api-core version is released, as the fix here will not have any effect on its own, and will not close the issue yet.

cc: @pradn

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 requested a review from crwilcox May 22, 2020 11:42
googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 22, 2020

plamut commented May 22, 2020

Copy link
Copy Markdown
Contributor Author

The docs failure seems to be unrelated to the actual change, it was just revealed now that we re-generated the files from protos.

dhendry commented Jun 2, 2020

Copy link
Copy Markdown

Please prioritize - we use pipenv and this compatibility problem has broken our development flow

plamut commented Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

@dhendry As of PubSub 1.4.3, the api-core dependency has been restricted to version < 1.17.0 and should not be experiencing this issue. Or is there another factor involved?

In any case, in order for this fix to work, a change also needs to be done (and released) in api-core. The change got a preliminary approval, and is now just awaiting some related check.

cc: @crwilcox Do you have free cycles to prioritize the check? Thanks!

dhendry commented Jun 3, 2020 •
edited
Loading

Copy link
Copy Markdown

@plamut We ran into problems because the latest version google-api-python-client (1.9.1) requires "google-api-core>=1.17.0,<2dev", which is incompatible with the google-cloud-pubsub restriction of < 1.17.0 (see https://github.com/googleapis/google-api-python-client/blob/v1.9.1/setup.py#L44)

For us google-api-python-client was being pulled in implicitly as a dependency of firebase-admin

plamut commented Jun 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@dhendry Ah, understood. The pip resolver should not allow that to be installed, but it currently has some bugs.

Anyhow, I will push for a new PubSub fix + release as soon as the api-core part of the fix is merged.

busunkim96 mentioned this pull request Jun 3, 2020
Closed

plamut commented Jun 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@dhendry (and others) As a temporary workaround, would it be feasible to pin and install an older version of google-api-python-client that does not require api-core >=1.17.0?

plamut requested a review from pradn June 4, 2020 09:07

plamut commented Jun 4, 2020

Copy link
Copy Markdown
Contributor Author

@pradn Another pair of eyes would be useful for the fix on the PubSub side.

I also removed all non-relevant synth changes from the PR to avoid the docs check issue due to mis-indented generated docstrings.

busunkim96 mentioned this pull request Jun 4, 2020
Merged
pradn suggested changes Jun 5, 2020
View reviewed changes
Comment thread google/cloud/pubsub_v1/gapic/subscriber_client.py
plamut added do not merge Indicates a pull request not ready for merge, due to either quality or timing. status: blocked Resolving the issue is dependent on other work. labels Jun 6, 2020

plamut commented Jun 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

We need to wait with merging until the api-core PR is merged and released, otherwise this fix will have no effect.

Even worse - it can install one of the more recent api-core releases that do not have the required fix yet. We will have to update the api-core version pin when it's clear which version range should be excluded.

arithmetic1728 commented Jun 9, 2020

Copy link
Copy Markdown
Contributor

@plamut The api-core PR is merged and just released (v1.20.0).

plamut removed the status: blocked Resolving the issue is dependent on other work. label Jun 9, 2020

pradn commented Jun 9, 2020

Copy link
Copy Markdown
Contributor

We're going to try to get this in for release 1.6.0.

plamut commented Jun 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@pradn Please just wait until I update version pins.

Update: Done, 1.20.0 is the first api-core version that includes the other part of the fix. All others from 1.17.0 on are banned.

plamut removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 9, 2020
plamut requested a review from pradn June 9, 2020 21:46
pradn suggested changes Jun 9, 2020
View reviewed changes
Comment thread setup.py Outdated
plamut requested a review from pradn June 9, 2020 22:16
pradn approved these changes Jun 9, 2020
View reviewed changes

pradn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perfect!

plamut merged commit c02060f into googleapis:master Jun 9, 2020
plamut deleted the iss-93 branch June 9, 2020 22:25
gcf-merge-on-green Bot pushed a commit that referenced this pull request Jun 9, 2020
chore: release 1.6.0 (#117)
60fe71a 
## [1.6.0](https://www.github.com/googleapis/python-pubsub/compare/v1.5.0...v1.6.0) (2020-06-09)

### Features

* Add flow control for message publishing ([#96](https://www.github.com/googleapis/python-pubsub/issues/96)) ([06085c4](https://www.github.com/googleapis/python-pubsub/commit/06085c4083b9dccdd50383257799904510bbf3a0))


### Bug Fixes

* Fix PubSub incompatibility with api-core 1.17.0+ ([#103](https://www.github.com/googleapis/python-pubsub/issues/103)) ([c02060f](https://www.github.com/googleapis/python-pubsub/commit/c02060fbbe6e2ca4664bee08d2de10665d41dc0b))


### Documentation
- Clarify that Schedulers shouldn't be used with multiple SubscriberClients ([#100](#100)) ([cf9e87c](cf9e87c))
- Fix update subscription/snapshot/topic samples ([#113](#113)) ([e62c38b](e62c38b))


### Internal / Testing Changes
- Re-generated service implementaton using synth: removed experimental notes from the RetryPolicy and filtering features in anticipation of GA, added DetachSubscription (experimental) ([#114](#114)) ([0132a46](0132a46))
- Incorporate will_accept() checks into publish() ([#108](#108)) ([6c7677e](6c7677e))
release-please Bot mentioned this pull request Aug 11, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

crwilcox Awaiting requested review from crwilcox
1 more reviewer
pradn pradn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Resolve Incompatibility with API Core >= 1.17.0

5 participants

Back | FazBrowse Home | New Git URL