fix(redis): stream read from Redis #830
Conversation
deverton
force-pushed
the
mem-reduce
branch
2 times, most recently
from
dcc184f
to
685e4ae
Compare
| log.error("Storage exception reading all entries.", e); | ||
| } | ||
| return null; | ||
| private RawUserPermission getRawUserPermissionsFromRedis(String userId) { |
There was a problem hiding this comment.
There's a lot of overlap between this and getFromRedis that it would be nice to consolidate
|
I like the approach - had a comment about the overlap of Some of the things in |
|
In testing so far we've not seen any problems with Redis (well, Elasticache) keeping up with the command rate but I understand the point. Combining the two functions would be ideal, but then what time out would apply during a sync process? Do you timeout the whole sync? Or just the individual value being synced at the time? |
This change makes the role sync process stream and process the data from Redis incrementally. Previously it would cache the entire data set before processing which would cause `fiat` to consume a large amount of heap on some data sets. May fix spinnaker/spinnaker#6328 and fix spinnaker/spinnaker#6319
This change shares the read from Redis code between `get()`, `getAllById()`, and `getAllByRoles()`. Impact to performance should be minimal as the unrestrictied user permissions cache is now used in the `getAllBy*` cases. There is however an additional `SISMEMBER` call per-user fetched rather than a single `SSCAN` and test on the Java side.
|
Updated the PR to unify the methods. This deletes a lot of code so I'm a bit suspicious about it still being correct, but in the testing I've done it seems fine. There's no more batching of user fetches though which seems like it should slow things down but doesn't seem to. It does open up the possibility of using |
This change makes the role sync process stream and process the data from Redis incrementally. Previously it would cache the entire data set before processing which would cause `fiat` to consume a large amount of heap on some data sets. May fix spinnaker/spinnaker#6328 and fix spinnaker/spinnaker#6319 This change shares the read from Redis code between `get()`, `getAllById()`, and `getAllByRoles()`. Impact to performance should be minimal as the unrestrictied user permissions cache is now used in the `getAllBy*` cases. There is however an additional `SISMEMBER` call per-user fetched rather than a single `SSCAN` and test on the Java side.
|
To follow up, we've been running this change in production for a while now and the memory reduction is pretty reasonable though it really only affects the peak usage.
|
This change makes the role sync process stream and process the data from Redis incrementally. Previously it would cache the entire data set before processing which would cause
fiatto consume a large amount of heap on some data sets.May fix spinnaker/spinnaker#6328 and fix spinnaker/spinnaker#6319