feat(sdk): Add Client::optimize_stores #5911
Conversation
jmartinesp
force-pushed
the
misc/add-client-optimize-stores-method
branch
from
a40c0e6 to
44514c3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5911 +/- ##
==========================================
- Coverage 88.65% 88.57% -0.09%
==========================================
Files 363 363
Lines 104737 104838 +101
Branches 104737 104838 +101
==========================================
+ Hits 92858 92860 +2
- Misses 7524 7625 +101
+ Partials 4355 4353 -2 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5911 will not alter performanceComparing Summary
|
jmartinesp
force-pushed
the
misc/add-client-optimize-stores-method
branch
from
44514c3 to
20f1921
Compare
jmartinesp
force-pushed
the
misc/add-client-optimize-stores-method
branch
3 times, most recently
from
876bbf8 to
7a81959
Compare
|
I kept the |
poljar
left a comment
poljar
left a comment
There was a problem hiding this comment.
I think there's a panic in there.
crates/matrix-sdk/src/client/mod.rs
Outdated
| self.state_store().optimize().await?; | ||
|
|
||
| trace!("Optimizing event cache store..."); | ||
| self.event_cache_store().lock().await?.as_clean().unwrap().optimize().await?; |
There was a problem hiding this comment.
Won't this panic in case the event cache lock is dirty and we try to optimize the store?
There was a problem hiding this comment.
Oops. Fixed in 9eb04ed using the same check than for the get_store_sizes function instead.
There was a problem hiding this comment.
The as_clean() is supposed to be used only for testing. It's better to pattern-match the lock state.
I think it's fine to keep it hidden for now. Not that it matters one way or the other since only people that implement the traits will be interested in this and they will have to know about it. |
This method should trigger any optimization/maintenance behaviours available to the stores, like `VACUUM` in SQLite
…raits Only SQLite based stores will implement it for now, calling the `SqliteAsyncConnExt::vacuum` method
…uccessfully This was a bit confusing, because I treated a lack of logs as success when in reality my code was calling an empty implementation
… in production Also fix lint issue
This method will retrieve the database sizes if available and expose it in the client. Note: the actual database size measuring is only implemented for the SQLite based stores
jmartinesp
force-pushed
the
misc/add-client-optimize-stores-method
branch
from
9eb04ed to
152405a
Compare
| } | ||
|
|
||
| /// Returns the sizes of the existing stores, if known. | ||
| pub async fn get_store_sizes(&self) -> Result<StoreSizes, ClientError> { |
There was a problem hiding this comment.
We usually don't have a get_ prefix, only a set_ prefix. It's not the end of the world, but so you know, it's more Rust idiomatic :-).
Added methods to perform cleanup/maintenance/optimization actions in the stores, so we can manually trigger them from the clients. This is a first attempt to understand if running these optimizations fixes an observed issue in the clients where the DB performance degrades severely as the cache grows.
Also added another set of methods to calculate the store sizes, when possible (
Client::get_store_sizes+*Store::get_size).cc @Hywan
Signed-off-by: