base: Move media cache to new EventCacheStore trait#3858
base: Move media cache to new EventCacheStore trait#3858bnjbvr merged 4 commits intomatrix-org:mainfrom
EventCacheStore trait#3858Conversation
d180536 to
7d963ad
Compare
bacba74 to
ec7060b
Compare
Allows to save media in a different path than the state store. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3858 +/- ##
=======================================
Coverage 84.09% 84.10%
=======================================
Files 261 266 +5
Lines 27622 27689 +67
=======================================
+ Hits 23229 23287 +58
- Misses 4393 4402 +9 ☔ View full report in Codecov by Sentry. |
|
Thanks, I'll take over the review. |
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
EventCacheStore trait
|
As discussed in the matrix room, I renamed the trait to |
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks! Would you be ok implementing the last access feature as a separate PR? I don't think it should be implemented only for the sqlite backend, and my proposal below should make it possible to implement for other backends too by tweaking the APIs.
crates/matrix-sdk-base/src/event_cache_store/integration_tests.rs
Outdated
Show resolved
Hide resolved
| let uri = self.encode_key(keys::MEDIA, request.source.unique_key()); | ||
| let format = self.encode_key(keys::MEDIA, request.format.unique_key()); | ||
| let data = self.encode_value(content)?; | ||
| self.acquire().await?.set_media(uri, format, data).await |
There was a problem hiding this comment.
Out of curiosity, what's the reasoning behind having multiple layers to perform SQL queries? (i.e. having set_media somewhere else, and not inlined here)
In my recent works on the send queue, I've put all the code at the same layer, and it feels simpler to understand, and with fewer indirections.
There was a problem hiding this comment.
I am not sure anymore. I mostly copy-pasted here what was in the StateStore implementation so we can definitely reduce the number of layers.
There was a problem hiding this comment.
Good, we can do this in a follow-up PR!
| } | ||
|
|
||
| #[async_test] | ||
| async fn test_last_access() { |
There was a problem hiding this comment.
Maybe we can introduce this feature in another PR, because I'd really like us to have it for all the EventCacheStore implementations, if we decide to go further down that road, and not only for a single backend, otherwise it may show at the use sites of the (abstract) event cache store.
Also I think for simplifying testing purposes and consistency of the backends, the timestamps should be provided by the caller; this will make it trivial to emulate time has spent without relying on tokio::time in tests, and will make it possible to implement this feature on the memory backend trivially too, I think.
There was a problem hiding this comment.
It is planned to be for all the implementations in the next PR. The reasons I introduced it like that for the SQLite store is that:
- This is a brand new database and I know we'll need this column so it seemed like a waste to init the table without this only to have to write a migration in the next PR.
- Even if we only add the column without more logic, if a client uses that commit before the next PR is merged, the data will be wrong unless we decide to write a migration to clear the database.
So this is basically a placeholder implementation while we wait for the next PR.
Of course I didn't do this for the memory store because it's not persistent so we can change anything without migration.
|
|
||
| tracing::info!("using event cache store @ {}", tmpdir_path.to_str().unwrap()); | ||
|
|
||
| Ok(SqliteEventCacheStore::open(tmpdir_path.to_str().unwrap(), None).await.unwrap()) |
There was a problem hiding this comment.
It seems sqlite supports in-memory backends too, and our code half supports it (the path is optional); is this something we could use in testing contexts, instead of having to create temporary directories and counting the number of temporaries we've created?
There was a problem hiding this comment.
Where do you see the path as optional in our code?
We could use in-memory stores, but currently we rely heavily on deadpool-sqlite that requires a file. It's fine if we want to use in-memory store but we have to rewrite this to be able to interact with both deadpool-sqlite and rusqlite directly. In that case the layers we currently have (or something similar) might make more sense.
There was a problem hiding this comment.
Where do you see the path as optional in our code?
path in the SqliteEventCacheStore impl; but it's probably a copy-pasto from the state store, which… never fills this path, and only uses it for debug purposes 🤦
Allows to save media in a different path than the state store.
This adds a "last_access" field to the SQLite implementation, to prepare for future work on a media retention policy.
This removes the IndexedDB media cache implementation, because as far as I know it is currently unused, and I have no idea how to implement efficiently the planned media retention policy with a key-value store.
Closes #1810.