Added support for Redis Cluster and Redis Sentinel #2961

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.

Sign up for GitHub

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

Jump to bottom

Merged

pracucci merged 23 commits into cortexproject:master from dmitsh:ds-redis-plus

Sep 14, 2020

Contributor

dmitsh commented Jul 30, 2020

Signed-off-by: Dmitry Shmulevich dmitry.shmulevich@sysdig.com

What this PR does:
Added support for Redis Cluster and Redis Sentinel

Which issue(s) this PR fixes:
Fixes #2959

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pull-request-size bot added the size/XXL label

pstibrany suggested changes

View reviewed changes

Contributor

pstibrany left a comment •

edited

Loading

Thank you! I don't have any Redis knowledge, but the code mostly makes sense to me. I've left few minor comments, if you could please take a look.

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_cache.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_cache.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

dmitsh changed the title ~~Added support for Redis Cluster and Redis Sentinel (#2959)~~

dmitsh requested a review from pstibrany

July 31, 2020 16:36

dmitsh force-pushed the ds-redis-plus branch from e431600 to c74e4af Compare

July 31, 2020 16:43

pracucci reviewed

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

dmitsh force-pushed the ds-redis-plus branch from d10fc8f to 50c79fc Compare

August 6, 2020 02:13

dmitsh requested a review from pracucci

August 6, 2020 02:29

pstibrany approved these changes

View reviewed changes

Contributor

pstibrany left a comment

Thanks for your work on this PR! I left some small comments about our flag names consistency, but otherwise looks good to me.

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

dmitsh force-pushed the ds-redis-plus branch from 1c35cc5 to b957139 Compare

August 10, 2020 18:42

pracucci reviewed

View reviewed changes

Contributor

pracucci left a comment

Solid work! 👏 I left few comments / questions, but overall looks good. I would appreciate if you could take a look at the comments and address them (unless you have any objection) so that we can move forward merging this. Thanks! 🚀

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Show resolved Hide resolved

pkg/chunk/cache/redis_cache_test.go Outdated Show resolved Hide resolved

dmitsh force-pushed the ds-redis-plus branch from b957139 to 2684d1d Compare

August 12, 2020 22:11

dmitsh requested a review from pracucci

August 13, 2020 00:34

pracucci reviewed

View reviewed changes

Contributor

pracucci left a comment

Thanks @dmitsh for addressing my feedback! Much appreciated! Changes LGTM (I left a couple of last minor comments), but I'm still concerned by opening a new TCP connection for every single cache operation when using sentinel. I can understand that the "protocol" requires you to get the master from sentinel each time, but we should probably keep the master clients in a reuseable pool, so that we just reuse an existing client (if any).

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

pracucci reviewed

View reviewed changes

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

dmitsh force-pushed the ds-redis-plus branch from 6ef9f0e to b6e461a Compare

August 20, 2020 06:10

dmitsh requested a review from pracucci

August 20, 2020 16:26

pracucci requested a review from pstibrany

August 27, 2020 15:35

dmitsh force-pushed the ds-redis-plus branch from 66c369f to b5c9f26 Compare

August 28, 2020 19:45

Contributor Author

dmitsh commented Aug 28, 2020

@pracucci @pstibrany , I've pushed another update. Please take a look.

pstibrany approved these changes

View reviewed changes

Contributor

pstibrany left a comment

Thank you, and apologies it took so long to review. Updating code to use "Universal Client" makes it so much simpler.

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

dmitsh requested a review from pstibrany

September 7, 2020 15:11

pstibrany approved these changes

View reviewed changes

Contributor

pstibrany left a comment

Thank you very much for all your work on this PR! I've left one additional comment to fix (and I apologize for missing it before), but other than that I think this is good to go.

pkg/chunk/cache/redis_client.go Outdated Show resolved Hide resolved

dmitsh requested a review from pstibrany

September 7, 2020 17:47

pstibrany approved these changes

View reviewed changes

dmitsh force-pushed the ds-redis-plus branch from 1d43eb1 to e85ffd5 Compare

September 11, 2020 16:38

Dmitry Shmulevich added 4 commits

September 14, 2020 15:16


          Added support for Redis Cluster and Redis Sentinel (cortexproject#2959)

d2f141b

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          addressed comments

5ea34fe

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          fixed 'make doc'

fbbb1ab

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          fixed 'make lint'

090a106

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>

Dmitry Shmulevich added 18 commits

September 14, 2020 15:16


          updated Changelog

b8c5f21

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated Changelog

a18f9f6

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated go.mod

c82eb62

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          addressed comments

62e74a4

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          removed deprecated flags in redis config

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated modules

6182d9f

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated modules

bbf44a4

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          added warning when Redis sentinel returns unexpected master info

7d97c27

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          fixed 'make lint'

a3fb232

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated unit test

72bde36

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          added master group name to Redis Sentinel config

54a6ac3

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated config validation

5fd7fe6

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          use redis universal client

6e4cc9e

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated dependencies

da78155

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          remove obsolete interface

5e762f6

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          addressed comments

19ab83b

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          add Redis DB index selection

df49404

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>


          updated CHANGELOG

5b34973

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>

pracucci force-pushed the ds-redis-plus branch from e85ffd5 to 5b34973 Compare

September 14, 2020 13:17


          Fixed CHANGELOG

8ea1965

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci approved these changes

View reviewed changes

Contributor

pracucci left a comment

LGTM, thanks!

pracucci merged commit cea6d0f into cortexproject:master

dmitsh deleted the ds-redis-plus branch

September 14, 2020 16:08

slim-bean mentioned this pull request

cortex redis client fails to connect #3290

Closed

kamijin-fanta mentioned this pull request

redis cluster not supported #4053

Closed

2 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment