-
Notifications
You must be signed in to change notification settings - Fork 514
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
Add support for Redis caching #354
Add support for Redis caching #354
Conversation
Created a WIP PR with a working version... but I've a lot of things todo for this to be usable and reviewable :) |
9515d79
to
52f28b9
Compare
- Add Redis client - Add Redis support - Add Redis file Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
52f28b9
to
3858ed2
Compare
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.
I've left some comments @dgzlopes, thanks for your work on the redis support. Also, sorry for a late review to go a different route, but feel free to reach out on DM to discuss further :)
@@ -0,0 +1,147 @@ | |||
package redis |
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.
There seems to be a ton of code that's pretty much the same as memcached/cache.go .. This can get tough to maintain. I would suggest we split out the cache section into a separate module and pass it a Cache interface which looks something like this
type Cache interface {
func get(ctx context.Context, key string) []byte
func set(ctx context.Context, key string, val []byte)
}
The different cache backends can then implement this small interface instead of the entire backend.Reader
/ backend.Writer
interfaces. It will also help to keep all the tests in one place.
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.
diskcache
should live inside this separate module too? Probably not, I suppose 😕
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.
diskcache
should also live there, but if that turns out to be tough we can always address it in a different PR :)
if cfg.Redis != nil { | ||
r, w, err = redis.New(r, w, cfg.Redis, logger) | ||
|
||
if err != nil { | ||
return nil, nil, nil, err | ||
} | ||
} | ||
|
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.
This can also be addressed by the other comment ^
Also, the config section for cache is getting a little untidy, can we add a new field like we have for backend
(Line 128) which indicates which cache type we are trying to use? And then we can add these in a switch case like above.
- Add Redis client - Add Redis support - Add Redis file Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
3858ed2
to
212ce13
Compare
Signed-off-by: Daniel González Lopes <[email protected]>
Signed-off-by: Daniel González Lopes <[email protected]>
object: tt.readerObject, | ||
} | ||
mockW := &mockWriter{} | ||
mr, _ := miniredis.Run() |
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.
A simple map cache can be implemented like in memcached tests but I saw Cortex is using this too, so OK.
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, thanks for your work @dgzlopes! 🎉
Signed-off-by: Daniel González Lopes <[email protected]>
One comment for the docs, even though this PR is merged. |
Maybe I'm missing something... But I see a title here: https://github.com/grafana/tempo/blob/master/docs/tempo/website/configuration/redis.md. I've to add it to some other place? I'm happy to move everything to sentence case. I used lower case... Because I followed what we have for S3 -> https://github.com/grafana/tempo/blob/master/docs/tempo/website/configuration/s3.md 😄 |
What this PR does:
This PR adds support for Redis caching on Tempodb.
TODO:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]