Skip to content

Pin Redis-py version#946

Merged
thedrow merged 1 commit into
celery:masterfrom
ashb:patch-1
Nov 15, 2018
Merged

Pin Redis-py version#946
thedrow merged 1 commit into
celery:masterfrom
ashb:patch-1

Conversation

@ashb

@ashb ashb commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Nov 15, 2018

Copy link
Copy Markdown

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.

@thedrow

thedrow commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

Thanks!
Keep me posted if redis/redis-py#1068 is resolved.

@ashb

ashb commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

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

ashb commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

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

thedrow commented Nov 16, 2018

Copy link
Copy Markdown
Contributor

I'll try to get to it.

@ashb

ashb commented Nov 16, 2018

Copy link
Copy Markdown
Contributor Author

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
thedrow added a commit that referenced this pull request Nov 19, 2018
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
thedrow pushed a commit that referenced this pull request Nov 19, 2018
Further to #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 #946
thedrow added a commit that referenced this pull request Nov 19, 2018
thedrow pushed a commit that referenced this pull request Dec 6, 2018
Further to #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 #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.

2 participants