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

Be able to use Redis sentinel #181

Closed
wants to merge 1 commit into from
Closed

Be able to use Redis sentinel #181

wants to merge 1 commit into from

Conversation

sbrunner
Copy link
Contributor

To be able to use a Redis cluster we need that dogpile.cache support to use the Redis sentinels.

Thanks in advance.

@zzzeek
Copy link
Member

zzzeek commented Jun 26, 2020

Hi there and thanks for making this proposal.

For this to be considered for inclusion I would need the following changes:

  1. Create a subclass of RedisBackend called SentinelRedisBackend which contains the additional parameters specific to the sentinel feature.

  2. inside the file dogpile/cache/backends/__init__.py, add a register_backend for this new subclass called "dogpile.cache.redis_sentinel"

  3. add a class to tests/cache/test_redis_backend.py that includes this backend so that it can be run through the test suite. while we don't have to necesarily have a test that runs with two separate redis services, if this new suite could at least include a test or two that asserts the arguments are passed through correctly, that would be helpful.

  4. Replace all occurrences of the phrases "master" / "slave" with "primary" / " replica".

Thanks!

@sbrunner
Copy link
Contributor Author

1,2. I will do that.
3. I didn't see how it's documented how to start the unit test, I think that we should first start a redis?
4. for me, it looks strange to have
primary = sentinel.master_for(service_name)
replica = sentinel.slave_for(service_name)
But I will do that.

Finally, I just have a question for 3., thanks in advance

@sbrunner
Copy link
Contributor Author

for 1. isn't RedisSentinelBackend a better name?

@zzzeek
Copy link
Member

zzzeek commented Jun 29, 2020

for 1. isn't RedisSentinelBackend a better name?

fine by me

@zzzeek
Copy link
Member

zzzeek commented Jun 29, 2020

1,2. I will do that.
3. I didn't see how it's documented how to start the unit test, I think that we should first start a redis?

dogpile uses tox for unit tests, so in this case you can run the redis -level tests as:

tox -e redis

if you have the redis-server binary installed in a standard distribution location it will be run automatically by pifpaf for the duration of the test suite. if not, the tests will gracefully degrade into a skip, so you want to make sure they come out as green/PASSED.

  1. for me, it looks strange to have
    primary = sentinel.master_for(service_name)
    replica = sentinel.slave_for(service_name)
    But I will do that.

this is not just my own preference it comes down from my company as well, do what you can and i appreciate it, I may go try to send a PR to the redis package to have the alternate terminology available at least

Finally, I just have a question for 3., thanks in advance

@sbrunner
Copy link
Contributor Author

Thanks @zzzeek, for me everything is done :-)

@zzzeek
Copy link
Member

zzzeek commented Jun 30, 2020

OK I"m out of town for a couple of days so I'll try to get back to this later in the week.

tox.ini Show resolved Hide resolved
@zzzeek zzzeek requested a review from sqla-tester July 3, 2020 15:37
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision b6d8e1e of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change b6d8e1e: https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2069

@sbrunner
Copy link
Contributor Author

sbrunner commented Jul 6, 2020

I also fix the pep8 issues :-)

@zzzeek
Copy link
Member

zzzeek commented Jul 6, 2020

OK graet, I haven't even looked at this, thanks for checking.

@zzzeek zzzeek requested a review from sqla-tester July 6, 2020 14:05
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 68a62c7 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 68a62c7 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2069

@sbrunner
Copy link
Contributor Author

sbrunner commented Jul 8, 2020

Hello, can Gerrit review it again?

@zzzeek
Copy link
Member

zzzeek commented Jul 8, 2020

hi, sure, I think github does not send me a notification when you push new changes, sorry.

@zzzeek zzzeek requested a review from sqla-tester July 8, 2020 13:36
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 28863ad of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 28863ad added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2069

@sbrunner
Copy link
Contributor Author

sbrunner commented Jul 8, 2020

@zzzeek no problem, thanks :-)

@zzzeek zzzeek requested a review from sqla-tester July 8, 2020 14:02
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision eb34335 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset eb34335 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2069

@zzzeek
Copy link
Member

zzzeek commented Jul 8, 2020

OK I did see that one in my email, so I guess I'm wrong :)

@zzzeek
Copy link
Member

zzzeek commented Jul 12, 2020

hey are you seeking python 2 support for this PR also? we are talking about the next dogpile release being python3 only but this is not decided.

@sbrunner
Copy link
Contributor Author

No, I don't use Python 2 anymore ...

@zzzeek
Copy link
Member

zzzeek commented Jul 13, 2020

ok iwill get this merged

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+2

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2069

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2069 has been merged. Congratulations! :)

@sbrunner
Copy link
Contributor Author

Thanks :-)

@sbrunner sbrunner deleted the redis-sentinel branch July 14, 2020 08:14
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.

None yet

3 participants