Skip to content
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): enable persistent storage by default 😎 #4308

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

bnjbvr
Copy link
Member

@bnjbvr bnjbvr commented Nov 21, 2024

This implements the following parts of #3280 (comment):

  • implement a sqlite backend for the event cache store
  • save events into the store as they come from sync
  • reload data from the store and use it to prefill the linked chunks for all rooms

For getting a nice demo in the multiverse client, a few shortcuts have been taken in the last commit, but these should be relatively easy to solve.

The demo shows the following:

  • open the client for the first time
  • observe the focused room is mostly empty
  • I manually back-paginate, getting some events back from the server
  • then, close the client
  • reopen it
  • observe the events are still there (they were persisted on disk).
Screencast.From.2024-11-21.18-09-05.mp4

More to come next week.

@bnjbvr
Copy link
Member Author

bnjbvr commented Nov 28, 2024

For what it's worth, to observers of this PR: this is being split, redesigned and reviewed in several smaller PRs, which all link to this one. This PR might be repurposed to represent the final step of the work done here, at some point.

@Hywan
Copy link
Member

Hywan commented Jan 10, 2025

Is it useful to keep this PR open?

@bnjbvr
Copy link
Member Author

bnjbvr commented Jan 13, 2025

Is it useful to keep this PR open?

I think I can repurpose it to become the "enable storage by default", by removing a few commits in there :-)

@bnjbvr bnjbvr changed the title feat(event cache): sqlite persistent storage for the event cache 😎 feat(event cache): enable the persistent storage for the event cache by default 😎 Jan 13, 2025
@bnjbvr bnjbvr changed the title feat(event cache): enable the persistent storage for the event cache by default 😎 feat(event cache): enable persistent storage by default 😎 Jan 13, 2025
@bnjbvr bnjbvr force-pushed the bnjbvr/sqlite-event-cache branch from 1473d60 to e9bc43a Compare January 13, 2025 13:29
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.14%. Comparing base (4742aa2) to head (b7f6b19).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/event_cache/pagination.rs 89.47% 4 Missing ⚠️
crates/matrix-sdk/src/event_cache/room/mod.rs 95.23% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #4308    +/-   ##
========================================
  Coverage   86.14%   86.14%            
========================================
  Files         291      291            
  Lines       34308    34165   -143     
========================================
- Hits        29553    29431   -122     
+ Misses       4755     4734    -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bnjbvr bnjbvr force-pushed the bnjbvr/sqlite-event-cache branch from e9bc43a to 07516cf Compare February 26, 2025 09:21
And don'y rely on the `Paginator`. This simplifies the code a bit,
avoids a few methods on the `Paginator`, and makes it more
straightforward the pagination happens.
Also the tests, they were not quite useful to port to the new mechanism
because they made little sense.
@bnjbvr bnjbvr force-pushed the bnjbvr/sqlite-event-cache branch from 07516cf to ac496f3 Compare February 27, 2025 17:00
@bnjbvr bnjbvr force-pushed the bnjbvr/sqlite-event-cache branch from f5ccfe4 to b7f6b19 Compare February 27, 2025 17:25
@bnjbvr bnjbvr marked this pull request as ready for review February 27, 2025 17:54
@bnjbvr
Copy link
Member Author

bnjbvr commented Feb 27, 2025

This is now ready for review, as it passes all the tests on my machine (and hopefully in CI too!).

But we're likely not going to merge this any time soon, as it removes the feature toggle for the event cache's persisted storage. Instead, we'll likely keep the feature toggle for a bit (exact period TBD: one release? some weeks? something that we'll discuss internally), to avoid having to revert a large PR in case things go awfully bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants