-
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: add example of redis mget batching using @expo/batcher #199
Conversation
875dab5
to
54436dd
Compare
Codecov Report
@@ Coverage Diff @@
## main #199 +/- ##
=======================================
Coverage 96.17% 96.17%
=======================================
Files 81 81
Lines 2065 2065
Branches 245 245
=======================================
Hits 1986 1986
Misses 79 79
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.
Making the batcher tunable might be useful but shouldn't block this PR. We should wait until we see if batching helps at all.
Might want to add a test case where two entities are deleted at the same time. There should be one batched DEL sent to Redis, though the timing is tricky because both Postgres deletions need to complete during the same microtask tick, so maybe that's better as a unit test with more mocking.
private readonly mgetBatcher = new Batcher<string[], (string | null)[]>( | ||
this.batchMgetAsync.bind(this), | ||
{ | ||
maxBatchInterval: 0, |
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 the future, we might want to set a max batch size so that requests and responses don't become too big, though ideally we'd compute this size based on the number of unique keys to fetch which would require changes to the batcher API (e.g. a function that is passed in that the batcher calls to determine whether to run)
54436dd
to
418749d
Compare
Yep. This PR is mostly just an example for this open source repo. We'll do the actual implementation in the expo backend repo.
We don't batch the |
Why
Add example of how to coalesce mgets within the redis interface. This will allow multiple entity companion instances (multiple requests) to more efficiently fetch values from redis.
How
Use
@expo/batcher
to batchmgets
. The otherIRedis
methods are not batched and probably don't need to be since they aren't used as frequently.Test Plan
Run new test.