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 reconnect_attempts not working with Redis 4.8 #108

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

brucebolt
Copy link
Member

Our apps are currently using Redis 4.8 due to a constraint from the version of Sidekiq we are using.

However in a previous commit, we used Redis 5 syntax to specify a new value for reconnect_attempts. This is not compatible with Redis 4.8.

Therefore updating the syntax to match that expected for Redis 4.8 and explicitly stating the version of Redis we expect (since Redis 5 doesn't support the legacy syntax).

Trello card

Our apps are currently using Redis 4.8 due to a constraint from the
version of Sidekiq we are using.

However in a previous commit, we used Redis 5 syntax to specify a new
value for `reconnect_attempts`. This is not compatible with Redis 4.8.

Therefore updating the syntax to match that expected for Redis 4.8 [1]
and explicitly stating the version of Redis we expect (since Redis 5
doesn't support the legacy syntax).

1: https://github.com/redis/redis-rb/tree/v4.8.1?tab=readme-ov-file#reconnections
@brucebolt brucebolt marked this pull request as ready for review June 11, 2024 10:28
Copy link
Contributor

@Gweaton Gweaton left a comment

Choose a reason for hiding this comment

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

Ah well spotted

@brucebolt brucebolt merged commit 3312287 into main Jun 11, 2024
17 checks passed
@brucebolt brucebolt deleted the fix-reconnect-attempts branch June 11, 2024 11:01
brucebolt added a commit that referenced this pull request Sep 10, 2024
This reverts the change made in
#108.
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