Skip to content

Add Redis support #1612

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
merged 37 commits into from
Oct 3, 2019
Merged

Add Redis support #1612

merged 37 commits into from
Oct 3, 2019

Conversation

dmitsh
Copy link
Contributor

@dmitsh dmitsh commented Aug 24, 2019

Adding support for Redis.
Reference: issue #1333

Dmitry Shmulevich added 2 commits August 23, 2019 18:31
adding support for Redis cache
e505d30 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
fix lint errors
54ffdb8 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
@dmitsh dmitsh mentioned this pull request Aug 24, 2019
Closed
Dmitry Shmulevich added 3 commits August 26, 2019 13:15
minor bugfix
97b2824 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
proper use of pool connections
3b57fc2 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
fix lint errors
bbaf6cc 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
@dmitsh dmitsh changed the title adding support for Redis cache Add Redis support Aug 29, 2019
@dmitsh dmitsh mentioned this pull request Sep 2, 2019
Closed
@blockloop blockloop mentioned this pull request Sep 3, 2019
Closed
@tomwilkie
Copy link
Contributor

Hi @dmitsh! This PR looks pretty good to me so far. I wonder why you've choosen to separate out the redis type from the redisClient type? I think they could be all one type.

@dmitsh
Copy link
Contributor Author

dmitsh commented Sep 5, 2019

@tomwilkie The main rationale was to follow memcached model. But I can see your point. Let me address that shortly.

Dmitry Shmulevich added 2 commits September 5, 2019 14:26
addressed comments
2dc96ec 
added unit test

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
Merge branch 'upstream-master' into ds-redis-cache
c7d3d89 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
@dmitsh
Copy link
Contributor Author

dmitsh commented Sep 5, 2019

@tomwilkie please take a look. I also added a unit test.

Dmitry Shmulevich added 3 commits September 5, 2019 16:03
update unit tests
b0b90de 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
reuse context in request timeout
a459387 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
use correct function names in logs
e94a4e0 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
bboreham
bboreham approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Code looks broadly OK, thanks for the PR!

I was a little surprised that it allows tiering of redis on top of memcached, which seems unlikely.
It's simpler to add redis as a whole separate thing to memcached, but I think the result is more hostile to users who end up with dozens of new options.
Similarly it would be good to combine the metrics rather than exporting two sets, of which one set is useless.

@bboreham
Copy link
Contributor

Oh, and you should add a line to CHANGELOG for the release.

Dmitry Shmulevich added 4 commits September 11, 2019 14:47
use common config for cache storage
ddac452 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
master rebase
2c8fa9e 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
added missing module dependency
03e30de 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
delete extra newline
6ec6c4d 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
@dmitsh
Copy link
Contributor Author

dmitsh commented Sep 11, 2019

@bboreham I've implemented the changes we discussed.

bboreham
bboreham approved these changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@bboreham bboreham requested a review from tomwilkie September 12, 2019 11:31
optimize redis SET
b9cc19b 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
gouthamve
gouthamve suggested changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

I don't think we should mix the cache configs together. We should separate them. We do the same in ring kv stores (consul, etcd) for example.

Dmitry Shmulevich added 2 commits September 18, 2019 10:39
adding support for Redis cache
efc0f0e 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
fix lint errors
52ded0f 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
Dmitry Shmulevich added 17 commits September 18, 2019 10:39
minor bugfix
b25f1e3 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
proper use of pool connections
e74f9d3 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
fix lint errors
1c1b925 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
addressed comments
121322a 
added unit test

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
update unit tests
724399c 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
reuse context in request timeout
1c1f1c4 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
use correct function names in logs
284cba0 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
use common config for cache storage
4d4a29b 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
added missing module dependency
476bd2a 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
delete extra newline
d0b5b85 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
optimize redis SET
9a60e34 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
undo common config for storage systems
4edda24 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
Merge branch 'ds-redis-cache' of github.com:dmitsh/cortex into ds-red…
2549dfe 
…is-cache

Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
Merge branch 'upstream-master' into ds-redis-cache
2ef6395 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
restore changes in CHANGELOG.md
4d03706 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
fixed lint error
7de661c 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
fixed mod-check error
11dc574 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
@dmitsh
Copy link
Contributor Author

dmitsh commented Sep 18, 2019 •
edited
Loading

@gouthamve per our discussion I have made requested changes.
I've also rebased the branch and updated dependencies.

Dmitry Shmulevich added 2 commits September 25, 2019 16:43
fix metric labels
9b2abaa 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
resolved git conflict
677bf6c 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
gouthamve
gouthamve approved these changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

@bboreham I spoke with Dmitry and we decided it is best to keep the specific config options separate. Right now it's cleaner as we're simply doing a {Memcached/Redis}Config.RegisterFlags and calling a NewRedisCache(RedisConfig).

If we had a common host and timeout, we'd be doing something similar to:

cfg.RedisConfig.Host = cfg.Host
cfg.RegisConfig.Timeout = cfg.Timeout

And similarly for Memcached. Further, having it this way would also mean we are backwards compatible and nothing needs to be deprecated.

Though we have -redis.hostname and memcached.hostname now, I think it's clearer to the users here.

Curious to know your thoughts on this.

bboreham
bboreham reviewed Oct 1, 2019
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Some initial thoughts.

addressed comments
f51f788 
Signed-off-by: Dmitry Shmulevich <dmitry.shmulevich@sysdig.com>
bboreham
bboreham approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

csmarchbanks
csmarchbanks approved these changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

LGTM

@csmarchbanks csmarchbanks merged commit 857bb84 into cortexproject:master Oct 3, 2019
@dmitsh dmitsh deleted the ds-redis-cache branch October 3, 2019 17:02
@dmitsh dmitsh restored the ds-redis-cache branch October 3, 2019 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csmarchbanks csmarchbanks csmarchbanks approved these changes

@gouthamve gouthamve gouthamve approved these changes

@bboreham bboreham bboreham approved these changes

@tomwilkie tomwilkie Awaiting requested review from tomwilkie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dmitsh @tomwilkie @bboreham @csmarchbanks @gouthamve