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

Pin Redis-py version #946

Merged
merged 1 commit into from
Nov 15, 2018
Merged

Pin Redis-py version #946

merged 1 commit into from
Nov 15, 2018

Conversation

ashb
Copy link
Contributor

@ashb ashb commented Nov 15, 2018

Redis-py 3.0.0 was released today and introduced a backwards incompatible changes.

https://github.com/andymccurdy/redis-py/blob/9b03af26dc829beea232a3248768de933f4c3b67/CHANGES#L27-L29

This isn't the "best" fix but is the quickest.

Redis-py 3.0.0 was released today and introduced a backwards incompatible changes.

https://github.com/andymccurdy/redis-py/blob/9b03af26dc829beea232a3248768de933f4c3b67/CHANGES#L27-L29

This isn't the "best" fix but is the quickest
@codecov
Copy link

codecov bot commented Nov 15, 2018

Codecov Report

Merging #946 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #946   +/-   ##
=======================================
  Coverage   88.66%   88.66%           
=======================================
  Files          63       63           
  Lines        6509     6509           
  Branches      776      776           
=======================================
  Hits         5771     5771           
  Misses        656      656           
  Partials       82       82

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e6fcca...afb2e12. Read the comment docs.

@ashb
Copy link
Contributor Author

ashb commented Nov 15, 2018

I think the interface to zadd in redis changed. This line from redis-py's changelog seems to cover it:

ZADD now requires all element names/scores be specified in a single
dictionary argument named mapping

I.e. I think

pipe.zadd(self.unacked_index_key, time(), delivery_tag)

needs to become

pipe.zadd(self.unacked_index_key, {time(): delivery_tag})

@ashb
Copy link
Contributor Author

ashb commented Nov 15, 2018

In the mean time what is the chance of getting a quick Kombu release out with this version pin included?
(I'm not sure what the Kombu release process looks like)

@ashb ashb deleted the patch-1 branch November 15, 2018 17:20
@ashb
Copy link
Contributor Author

ashb commented Nov 16, 2018

Could it be helpful if I opened a PR that fixes this properly and supports redis-py v2 and v2 concurrently? (Thinking about ease of upgrading for users of celery, who are often not installing kobu directly, or don't do pip install kombu[redis] so might get a different version?

ashb added a commit to ashb/kombu that referenced this pull request Nov 16, 2018
Further to celery#946 this fixes the underlying issue in a easy-to-upgrade way for end users, many of whom will have redis installed via other means. By having this check here and supporting both versions concurrently it makes it easier for end users, and to use celery/kombu in projects that use redis elsewhere.

With this change it is possibly worth reverting celery#946
@ashb ashb mentioned this pull request Nov 16, 2018
ashb added a commit to ashb/kombu that referenced this pull request Nov 18, 2018
Further to celery#946 this fixes the underlying issue in a easy-to-upgrade way
for end users, many of whom will have Redis installed via other means.
By having this check here and supporting both versions concurrently it
makes it easier for end users, and to use celery/kombu in projects that
use Redis elsewhere.

With this change it is possibly worth reverting celery#946
ashb added a commit to ashb/kombu that referenced this pull request Nov 19, 2018
Further to celery#946 this fixes the underlying issue in a easy-to-upgrade way
for end users, many of whom will have Redis installed via other means.
By having this check here and supporting both versions concurrently it
makes it easier for end users, and to use celery/kombu in projects that
use Redis elsewhere.

With this change it is possibly worth reverting celery#946
ashb added a commit to ashb/kombu that referenced this pull request Nov 19, 2018
Further to celery#946 this fixes the underlying issue in a easy-to-upgrade way
for end users, many of whom will have Redis installed via other means.
By having this check here and supporting both versions concurrently it
makes it easier for end users, and to use celery/kombu in projects that
use Redis elsewhere.

With this change it is possibly worth reverting celery#946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant