feat(redis): add redis implementation of KvBufferStore
#62
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #62 +/- ##
===========================================
+ Coverage 56.46% 56.56% +0.10%
===========================================
Files 18 18
Lines 5152 5164 +12
Branches 244 244
===========================================
+ Hits 2909 2921 +12
Misses 2241 2241
Partials 2 2 ☔ View full report in Codecov by Sentry. |
| depends_on: | ||
| - redis |
There was a problem hiding this comment.
we don't use this for other services, so may remove
There was a problem hiding this comment.
Eh, I think we should keep it here. It's more important for redis since the service will maintain a persistent connection to it. Might not hurt to add it for the other services too tbh.
| depends_on: | ||
| - redis |
There was a problem hiding this comment.
Eh, I think we should keep it here. It's more important for redis since the service will maintain a persistent connection to it. Might not hurt to add it for the other services too tbh.
src/store/redis-kv-store.ts
Outdated
| private client: RedisClientType; | ||
| private log: winston.Logger; | ||
|
|
||
| constructor({ log, redisUrl }: { log: winston.Logger; redisUrl: string }) { |
There was a problem hiding this comment.
We should add support for passing in a TTL here. I think we'll both want to bound the size of the cache in Redis and expire values with some reasonable TTL (maybe 8 hours).
| url: redisUrl, | ||
| }); | ||
| this.client.on('error', (err) => { | ||
| this.log.error(`Redis error: ${err}`); |
There was a problem hiding this comment.
Let's add a Prometheus metric for this too.
There was a problem hiding this comment.
will add - do you think it's worth adding metrics for cache hits/misses?
There was a problem hiding this comment.
Initial connection error is good to have, but not exactly what I was intending. I was wanting one in the on('error'... handler. Still fine to keep the connect error counter too.
There was a problem hiding this comment.
apologies - complete misread on my part. added in 2dcf902
| url: redisUrl, | ||
| }); | ||
| this.client.on('error', (err) => { | ||
| this.log.error(`Redis error: ${err}`); |
There was a problem hiding this comment.
Initial connection error is good to have, but not exactly what I was intending. I was wanting one in the on('error'... handler. Still fine to keep the connect error counter too.
This implementation is farily straightforward. We will add a `redis` container to the `docker-compose` and set is as the default `KvBufferStore` for header and transaction cache data once we validate it works as expected.
…ferStore` Promising through initial testing. Makes the `core` service dependent on `redis` container starting properly and mounts redis data to local volume. We may not want to default the `REDIS_CACHE_URL` but set to localhost for now.
Changing to set the default `KvBufferStore` to redis in the docker-compose and `fs` in the app. We cannot guranatee redis is running if the app is run as standalone, but can ensure `redis` is up and running in the `docker-compose`. Also updates the redis volume to use the proper default `data/` path where `rdb` files get saved. We may want to consider setting the `appendonly` flag when starting `redis`.
We can add an alert on this if redis is unable to connect to the provided url
…2GiB The actual intent was to track errors on redis, not just connection issues. Added another counter for the errors, and kept the connection counter since it is useful to monitor. Setting the max memory by default to 2GiB in the docker compose to constrain unintended memmory usage in the container, but added an `REDIS_MAX_MEMORY` if they would like to set it to something else.
It was incorrectly set causing the container not to start.
dtfiedler
force-pushed
the
PE-4924-redis-kv-store
branch
from
2dcf902
to
fb63585
Compare
There was an extra character in the default value for REDIS_CACHE_TTL_SECONDS.
djwhitt
force-pushed
the
PE-4924-redis-kv-store
branch
from
ae257a3
to
c16b23a
Compare
This implementation is farily straightforward. We will add a
rediscontainer to thedocker-composeand set is as the defaultKvBufferStorefor header and transaction cache data once we validate it works as expected.