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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Jun 9, 2020
Merged

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

merged 10 commits into from
Jun 9, 2020

Conversation

plamut
Copy link
Contributor

@plamut plamut commented May 22, 2020 •
edited
Loading

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

plamut commented May 22, 2020

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
Copy link

dhendry commented Jun 2, 2020

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

@plamut
Copy link
Contributor Author

plamut commented Jun 3, 2020

@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
Copy link

dhendry commented Jun 3, 2020 •
edited
Loading

@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
Copy link
Contributor Author

plamut commented Jun 3, 2020 •
edited
Loading

@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 busunkim96 mentioned this pull request Jun 3, 2020
Closed
@plamut
Copy link
Contributor Author

plamut commented Jun 4, 2020 •
edited
Loading

@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 plamut requested a review from pradn June 4, 2020 09:07
@plamut
Copy link
Contributor Author

plamut commented Jun 4, 2020

@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 busunkim96 mentioned this pull request Jun 4, 2020
Merged
@plamut 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
Copy link
Contributor Author

plamut commented Jun 6, 2020 •
edited
Loading

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
Copy link
Contributor

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

@plamut plamut removed the status: blocked Resolving the issue is dependent on other work. label Jun 9, 2020
@pradn
Copy link
Contributor

pradn commented Jun 9, 2020

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

@plamut
Copy link
Contributor Author

plamut commented Jun 9, 2020 •
edited
Loading

@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 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 plamut requested a review from pradn June 9, 2020 21:46
pradn
pradn requested changes Jun 9, 2020
View reviewed changes
setup.py Outdated Show resolved Hide resolved
@plamut plamut requested a review from pradn June 9, 2020 22:16
pradn
pradn approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@pradn pradn left a comment

Choose a reason for hiding this comment

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

Perfect!

@plamut plamut merged commit c02060f into googleapis:master Jun 9, 2020
@plamut 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
@release-please
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 release-please bot mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pradn pradn pradn approved these changes

@crwilcox crwilcox Awaiting requested review from crwilcox

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
@plamut @dhendry @arithmetic1728 @pradn @googlebot