-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: alternative sharded redis cache #197
Conversation
Codecov Report
@@ Coverage Diff @@
## main #197 +/- ##
=======================================
Coverage 96.17% 96.17%
=======================================
Files 81 81
Lines 2065 2065
Branches 269 245 -24
=======================================
Hits 1986 1986
- Misses 74 79 +5
+ Partials 5 0 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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 code here looks fine to me. I'm assuming the plan is to make a custom class that implements IRedis to do your sharding.
export interface IRedis { | ||
mget(...args: [...keys: string[]]): Promise<(string | null)[]>; | ||
multi(): IRedisTransaction; | ||
del(...args: [...keys: string[]]): Promise<any>; |
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.
del(...args: [...keys: string[]]): Promise<any>; | |
del(...args: [...keys: string[]]): Promise<number>; |
Integer reply: The number of keys that were removed.
private getRedisClientsForKeys( | ||
keys: readonly string[] | ||
): Map<number, { keys: string[]; redisClient: Redis }> { | ||
const shardGroupsForKeys = new Map(keys.map((k) => [k, getShardForKey(k)])); |
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.
Instead of shard groups, aren't these just shards? (Map<Key, Shard>)
return redisClientsForKeys; | ||
} | ||
|
||
private getRedisInstanceForShardGroup(shardGroup: number): 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.
The other places in the code refer to "Redis clients":
private getRedisInstanceForShardGroup(shardGroup: number): Redis { | |
private getRedisClientForShardGroup(shardGroup: number): Redis { |
return ret; | ||
} | ||
|
||
async del(...args: string[]): Promise<void> { |
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.
In a non-test implementation, this should return a number (the sum of the individual del results)
This was a proof of concept. No longer needed for now. |
Why
This is the alternative to #196 mentioned in #196 (comment).
How
Instead of doing the sharding logic in the entity cache adapter level, do it within the redis interface. This requires some bookkeeping and tricks within the multi-redis logic, but isn't horrendous.
The main thing to review is the
ShardedRedis
class in the test. This is what would need to be duplicated to an application that uses it (though with a real consistent hash).Test Plan
Run test (same test as other PR to prove it works).