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

Sentinel commands have migrated in redis main #542

Closed
sarathak opened this issue Aug 23, 2021 · 14 comments · Fixed by #576
Closed

Sentinel commands have migrated in redis main #542

sarathak opened this issue Aug 23, 2021 · 14 comments · Fixed by #576
Labels

Comments

@sarathak
Copy link
Contributor

Redis pre release test throwing error in CI

  • Test Python 3.9, Django 3.2, Redis.py master
  • Test Python 3.9, Django 3.2 main, Redis.py master
  self = Sentinel<sentinels=[127.0.0.1:26379]>, service_name = 'default_service'
  
      def discover_master(self, service_name):
          """
          Asks sentinel servers for the Redis master's address corresponding
          to the service labeled ``service_name``.
      
          Returns a pair (address, port) or raises MasterNotFoundError if no
          master is found.
          """
          for sentinel_no, sentinel in enumerate(self.sentinels):
              try:
  >               masters = sentinel.sentinel_masters()
  E               AttributeError: 'Redis' object has no attribute 'sentinel_masters'
  
  .tox/py39-dj32-redismaster/lib/python3.9/site-packages/redis/sentinel.py:211: AttributeError
  _____ ERROR at teardown of DjangoRedisCacheTestEscapePrefix.test_iter_keys _____
  
  self = <tests.test_backend.DjangoRedisCacheTestEscapePrefix testMethod=test_iter_keys>
  
      def tearDown(self):
@WisdomPill
Copy link
Member

hello @sarathak,
redis master is allowed to fail as there might be new unreleased changes that will break the library.
Have you had look at what this might be?
Are you open to make changes to solve this for newer version of redis-py?

@sarathak
Copy link
Contributor Author

@WisdomPill

my opinion is we have to remove pre release test from CI

@WisdomPill
Copy link
Member

why one earth would you want to do that?

pre releases is added for this very reason, so that we know that before releasing a new version we have to solve some issues first to support the next version of redis

@WisdomPill
Copy link
Member

In this case, sentinel and cluster commands have moved to it's own mixins, have a look here. New commands are found here

@WisdomPill WisdomPill changed the title CI error with pre-release redis Sentinel commands have migrated in redis main Aug 23, 2021
@chayim
Copy link

chayim commented Aug 29, 2021

We're planning on releasing a new redis-py in the short-term. This is a great catch that your project has in its pipeline, I highly recommend maintaining these tests. Note: We haven't broken the API itself, but I'd like to go down the path of more restructuring in the mid-term.

@WisdomPill
Copy link
Member

@chayim thanks for the notice.
Can you elaborate what kind of restructuring you have in mind? Just so we can get an idea of what and where do the changes.
Is this new version going to be a major release or not?

@chayim
Copy link

chayim commented Aug 29, 2021 via email

@WisdomPill WisdomPill added the bug label Sep 20, 2021
@WisdomPill
Copy link
Member

@chayim do you ming changing the version (here) of redis-py in master so that we could start adapting the code to handle the recent changes?

or I could open a PR to do it.

@chayim
Copy link

chayim commented Oct 11, 2021

@WisdomPill In prep for 4.0.0, I just pushed up a 3.9.9 placeholder version.

For anyone not pulling directly from master that's no effect (or release), but this should solve the ask. Also, thank you for the pointer about the commands migrating in redis main - I'm going to update the changelog accordingly for breaking changes.

@WisdomPill
Copy link
Member

Thank you very much!

@chayim
Copy link

chayim commented Oct 20, 2021

@WisdomPill they're about to migrate again now that the 4.0.0.beta1 is out. Just a heads up. Similarly I'm about to deprecate python 3.5 support for redis. I noticed that this, and celery both use python > 3.6 (as we should), but I wanted to give you a heads up.

@WisdomPill
Copy link
Member

Thanks for the heads up!

@hartwork
Copy link
Contributor

hartwork commented Dec 1, 2021

This issue is supposedly fixed by commit redis/redis-py@d2b2333 that is included in redis-py >=4.0.2 already today. The commit adds method Redis.sentinel_masters by making class Redis inherit from SentinelCommands where the related code resides.

As a result, blocking redis-py 4.0.2 in pull request #570 may cause more harm than good and keep users in the past with redis-py. Another user of redis-py — the kombu project — only blocks redis-py 4.0.0 and 4.0.1 for a week now, see https://github.com/celery/kombu/pull/1445/files .

What are your thoughts on unblocking redis-py 4.0.2 in django-redis?

CC hartwork/jawanndenn#357

hartwork added a commit to hartwork/django-redis that referenced this issue Dec 3, 2021
Upstream commit redis/redis-py@d2b2333
that is included with redis-py >=4.0.2 adds method Redis.sentinel_masters
by making class Redis inherit from SentinelCommands, where the related code resides.

Related to issue jazzband#542, follow-up to pull request jazzband#570
hartwork added a commit to hartwork/django-redis that referenced this issue Dec 3, 2021
Upstream commit redis/redis-py@d2b2333
that is included with redis-py >=4.0.2 adds method Redis.sentinel_masters
by making class Redis inherit from SentinelCommands, where the related code resides.

Related to issue jazzband#542, follow-up to pull request jazzband#570
@hartwork
Copy link
Contributor

hartwork commented Dec 3, 2021

PS: I have created a related pull request #576 for review just now.

hartwork added a commit to hartwork/django-redis that referenced this issue Dec 9, 2021
Upstream commit redis/redis-py@d2b2333
that is included with redis-py >=4.0.2 adds method Redis.sentinel_masters
by making class Redis inherit from SentinelCommands, where the related code resides.

Related to issue jazzband#542, follow-up to pull request jazzband#570
hartwork added a commit to hartwork/django-redis that referenced this issue Dec 9, 2021
…, jazzband#570)

Upstream commit redis/redis-py@d2b2333
that is included with redis-py >=4.0.2 adds method Redis.sentinel_masters
by making class Redis inherit from SentinelCommands, where the related code resides.

Related to issue jazzband#542, follow-up to pull request jazzband#570
This reverts (parts of) commit ef945d6.
terencehonles pushed a commit to WisdomPill/django-redis that referenced this issue Dec 13, 2021
…, jazzband#570)

Upstream commit redis/redis-py@d2b2333
that is included with redis-py >=4.0.2 adds method Redis.sentinel_masters
by making class Redis inherit from SentinelCommands, where the related code resides.

Related to issue jazzband#542, follow-up to pull request jazzband#570
This reverts (parts of) commit ef945d6.
WisdomPill pushed a commit that referenced this issue Dec 13, 2021
#576)

Upstream commit redis/redis-py@d2b2333
that is included with redis-py >=4.0.2 adds method Redis.sentinel_masters
by making class Redis inherit from SentinelCommands, where the related code resides.

Related to issue #542, follow-up to pull request #570
This reverts (parts of) commit ef945d6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants