Skip to content

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

Merged
plamut merged 10 commits intogoogleapis:masterfrom
plamut:iss-93
Jun 9, 2020
Merged

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

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
pradn
pradn requested changes Jun 5, 2020
View reviewed changes
@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

arithmetic1728 commented Jun 9, 2020

@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
@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://github.com/googleapis/python-pubsub/compare/v1.5.0...v1.6.0) (2020-06-09)

### Features

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


### Bug Fixes

* Fix PubSub incompatibility with api-core 1.17.0+ ([#103](https://github.com/googleapis/python-pubsub/issues/103)) ([c02060f](https://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