-
Notifications
You must be signed in to change notification settings - Fork 1k
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: hiredis requires df to report version >7.4 #4474
Conversation
@@ -26,3 +26,4 @@ pytest-icdiff==0.8 | |||
pytest-timeout==2.2.0 | |||
asyncio==3.4.3 | |||
fakeredis[json]==2.26.2 | |||
hiredis==2.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing this here, hiredis 3.0 is incompatible with redis 5.2 (latest release).
TODO bump up the versions once they are fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More on this redis/hiredis-py#189 and redis/redis-py#3358 (comment)
Signed-off-by: kostas <[email protected]>
Signed-off-by: kostas <[email protected]>
@@ -230,7 +230,7 @@ using strings::HumanReadableNumBytes; | |||
|
|||
namespace { | |||
|
|||
const auto kRedisVersion = "7.2.0"; | |||
const auto kRedisVersion = "7.4.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider exposing it in common.h as a constant (or other header file) and then reference it in tests reduce amount of manual changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used in dragonfly_test.cc
so not much we gain here. There rest are python changes because the newer versions of the lib deprecated some of the old functions.
I am merging this and will follow up on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
hiredis checks for redis version and requires to be greater than 7.4. Dragonfly reports 7.2 and the library throws an error.
Resolves #4473