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

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

Merged
merged 10 commits into from Jun 9, 2020
Merged

Conversation

@plamut
Copy link
Contributor

@plamut plamut commented May 22, 2020

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

@plamut 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.

Loading

@dhendry
Copy link

@dhendry dhendry commented Jun 2, 2020

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

Loading

@plamut
Copy link
Contributor Author

@plamut 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!

Loading

@dhendry
Copy link

@dhendry dhendry commented Jun 3, 2020

@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

Loading

@plamut
Copy link
Contributor Author

@plamut plamut commented Jun 3, 2020

@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.

Loading

@plamut
Copy link
Contributor Author

@plamut plamut commented Jun 4, 2020

@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?

Loading

@plamut plamut requested a review from pradn Jun 4, 2020
@plamut
Copy link
Contributor Author

@plamut 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.

Loading

Loading
pradn
pradn approved these changes Jun 5, 2020
@plamut
Copy link
Contributor Author

@plamut plamut commented Jun 6, 2020

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.

Loading

@arithmetic1728
Copy link
Contributor

@arithmetic1728 arithmetic1728 commented Jun 9, 2020

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

Loading

@pradn
Copy link
Contributor

@pradn pradn commented Jun 9, 2020

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

Loading

@plamut
Copy link
Contributor Author

@plamut plamut commented Jun 9, 2020

@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.

Loading

@plamut plamut requested a review from pradn Jun 9, 2020
setup.py Outdated Show resolved Hide resolved
Loading
@plamut plamut requested a review from pradn Jun 9, 2020
pradn
pradn approved these changes Jun 9, 2020
Copy link
Contributor

@pradn pradn left a comment

Perfect!

Loading

@plamut plamut merged commit c02060f into googleapis:master Jun 9, 2020
3 checks passed
Loading
@plamut plamut deleted the iss-93 branch Jun 9, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Jun 9, 2020
## [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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants