FIX: update Redis gem to version 4.1.3 (PR #8197)

I run our benchmark on commit with hiredis and redis-4.1.3

Results:

hidredis redis 4.1.3 percent
Categories-50 49 50 102.04%
Categories-75 51 51 100.00%
Categories-90 63 64 101.59%
Categories-99 86 85 98.84%
Home-50 55 55 100.00%
Home-75 56 57 101.79%
Home-90 68 69 101.47%
Home-99 102 104 101.96%
Topic-50 36 37 102.78%
Topic-75 37 37 100.00%
Topic-90 47 48 102.13%
Topic-99 60 61 101.67%
Categories-admin-50 124 117 94.35%
Categories-admin-75 130 129 99.23%
Categories-admin-90 147 143 97.28%
Categories-admin-99 204 199 97.55%
Home-admin-50 146 148 101.37%
Home-admin-75 150 152 101.33%
Home-admin-90 169 168 99.41%
Home-admin-99 232 223 96.12%
Topic-admin-50 60 61 101.67%
Topic-admin-75 64 63 98.44%
Topic-admin-90 76 73 96.05%
Topic-admin-99 124 94 75.81%
Load rails 2412 2360 97.84%
rss 290204 295828 101.94%
pss 277948 283624 102.04%

DEV: https://dev.discourse.org/t/getting-rid-of-hiredis-and-upgrading-redis-to-latest/17136

GitHub

You’ve signed the CLA, lis2. Thank you! This pull request is ready for review.

hmmm is this hiredis, I think you only upgraded redis here? hiredis was removed yesterday.

What about this error:

# holding off redis upgrade temporarily as it is having issues with our current
# freedom patch, we will follow this up.
#
# FrozenError: can't modify frozen Hash
# /var/www/discourse/vendor/bundle/ruby/2.5.0/gems/redis-4.1.0/lib/redis/client.rb:93:in `delete'
# /var/www/discourse/vendor/bundle/ruby/2.5.0/gems/redis-4.1.0/lib/redis/client.rb:93:in `initialize'
# /var/www/discourse/lib/freedom_patches/redis.rb:7:in `initialize'

Mentioned in the Gemfile. Is it no longer relevant?

@SamSaffron yes, I noticed that hiredis was removed. Nevertheless, because hiredis was introduced to improve performance I wanted to ensure it is still fine. I executed the first benchmark on last available commit with hiredis (0de7e4339c72c993835a30273752a12e4e956fb8)

I missed comment about freedom patch. That code with the fix was merged (https://github.com/redis/redis-rb/pull/591) - in a little bit different form, but I think we should try to get rid of freedom patch for Redis.

I run smoke tests on my local and it seems to be fine.

1 Like

This looks good to me! happy to have redis updated!