-
Notifications
You must be signed in to change notification settings - Fork 268
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(event cache): connect the room event cache and persistent storage #4347
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4347 +/- ##
==========================================
+ Coverage 85.13% 85.15% +0.02%
==========================================
Files 280 280
Lines 30666 30716 +50
==========================================
+ Hits 26107 26157 +50
Misses 4559 4559 ☔ View full report in Codecov by Sentry. |
Getting back to a draft for this; I suspect a bit more work is required because the back-pagination tokens ought to belong in this state too… |
c9eb2db
to
d6afbcc
Compare
Calling this ready for review, after a rebase from main. I think the backpagination issue I've thought about, should be resolved independently. @Hywan Whenever the time is right for you, PTAL |
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.
It looks good. I like the new API to mutate the state. Thanks for putting everything together.
.await | ||
.unwrap(); | ||
|
||
// The stream doesn't report these changes *yet*. Use the items vector given |
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.
It's part of the PR I'm preparing I suppose, with VectorDiff
being emitted by EventCache
?
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.
Exactly. With your PR, we'd get a remove(0) and a push_back (or insert(1) ^^). Right now, these are not sent via the updates, and that's why we can't use this by default yet 👀
…e (conditionally)
d6afbcc
to
fca4131
Compare
This implements both directions for interacting between the event cache and the storage backend. I think this is fine design… as long as we don't have any other process writing into the cache, in which case we'll need to reset at different places.
writing
The main idea in this PR is to use
RoomEventCacheState::with_events_mut(func: impl FnMut(&mut RoomEvents))
to perform any write to the underlyingRoomEvent
data structure. The function takes care of maintaining the storage, after updates have been performed.Additionally: this behavior isn't enabled by default; callers have to call
EventCache::enable_storage()
when they start using the event cache.I've put the
RoomEventCacheState
data structure in a private module, so that even the same module can't access its private fields, and we don't make mistakes by accidentally misusingevents
.reading
Reading happens by reloading the chunks initially, and prefilling the deduplicator with the reloaded events information. It's mostly boring changes at this point.
Part of #3280. On top of #4362. Split and reworked from #4308.