Add redis wrappers & sub config support #6
Conversation
JamesReate
changed the title
redis/redis_cache_api_service.go
Outdated
| TLSConfig: tlsConfig, | ||
| }) | ||
| // cluster client does not have setting for DB, so must set it manually. | ||
| cc.Do(context.Background(), fmt.Sprintf("SELECT %d", redisSettings.DB)) |
There was a problem hiding this comment.
Does this call succeed? From the docs:
When using Redis Cluster, the SELECT command cannot be used, since Redis Cluster only supports database zero.
I wonder if we're just putting everything in the same database without knowing it.
There was a problem hiding this comment.
yeah good find. no idea will have to look.
There was a problem hiding this comment.
gonna remove that and leave a comment. I had only tested in local and dev.
There was a problem hiding this comment.
It could be dangerous to have this isolation work in dev and not prod.
You could pass in a prefix to use for all the keys, does that help at all?
There was a problem hiding this comment.
hmm yeah, I was wondering how safe this would be, and yeah that is probably what we want to do. Also for administrative stuff, like finding all the keys for a specific service would be easier with a prepend search eg. KEYS dbNumb_* or for big sets SCAN 0 COUNT 50 MATCH dbNum_*
Proposed changes
Add redis wrappers that we're using in a different projects so as to help standardize and reduce copy paste code etc. Note that we have a prefix property to optionally prefix keys due to sharing redis cluster with different projects, since the DB number option is not supported in redis cluster mode.
allow sub settings configuration in config loader. I know there is a way to make this infinitely hierarchical but idk if that is something we should allow, nor did i want to spend the time doing that / testing it.