feat: Add MediaRetentionPolicy to the EventCacheStore, take 2 #4571
Conversation
This will be used as a configuration to decide whether or not to keep media in the cache, allowing to do periodic cleanups to avoid to have the size of the media cache grow indefinitely. Signed-off-by: Kévin Commaille <[email protected]>
zecakeh
force-pushed
the
media-retention-policy
branch
from
966a3e9 to
ff37e93
Compare
This is an API to handle the MediaRetentionPolicy with a lower level EventCacheStoreMedia trait. Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
It is already a 3-tuple and we want to add more data so it will be clearer to use a stuct with named fields. Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
zecakeh
force-pushed
the
media-retention-policy
branch
from
ff37e93 to
504a9d4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4571 +/- ##
==========================================
+ Coverage 85.42% 85.67% +0.25%
==========================================
Files 285 288 +3
Lines 32226 33150 +924
==========================================
+ Hits 27528 28402 +874
- Misses 4698 4748 +50 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kévin Commaille <[email protected]>
…ted changes Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
zecakeh
force-pushed
the
media-retention-policy
branch
from
6835778 to
7c4c359
Compare
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
There was a problem hiding this comment.
The commit message (fd7f88e) is a good basis for this module documentation :-].
| use ruma::time::{Duration, SystemTime}; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| /// The retention policy for media content used by the `EventCacheStore`. |
There was a problem hiding this comment.
Can we have an intra-link to EventCacheStore please?
/// … by the [`EventCacheStore`].
///
/// [`EventCacheStore`]: crate::event_cache::store::EventCacheStore
| } | ||
|
|
||
| impl MediaRetentionPolicy { | ||
| /// Create a `MediaRetentionPolicy` with the default values. |
There was a problem hiding this comment.
Intra-link:
- /// Create a `MediaRetentionPolicy` with
+ /// Create a [`MediaRetentionPolicy`] with| Self::default() | ||
| } | ||
|
|
||
| /// Create an empty `MediaRetentionPolicy`. |
| /// Create an empty `MediaRetentionPolicy`. | ||
| /// | ||
| /// This means that all media will be cached and cleanups have no effect. | ||
| pub fn empty() -> Self { |
There was a problem hiding this comment.
I propose to rename empty to infinite or unlimited, I guess it conveys the intent better, thoughts?
- pub fn empty() -> Self {
+ pub fn unlimited() -> Self {There was a problem hiding this comment.
In a future PR, I am planning to allow to launch periodic cleanup jobs. It will add a field that is not a limitation, but a setting of how often the cleanup jobs should run. If this struct does not contain only limitations, infinite and unlimited do not seem to fit.
| OpenStoreError, | ||
| }; | ||
|
|
||
| mod keys { | ||
| // Entries in Key-value store | ||
| pub const MEDIA_RETENTION_POLICY: &str = "media-retention-policy"; |
There was a problem hiding this comment.
Can we have _ instead of - please?
- "media-retention-policy"
+ "media_retention_policy"| let current_timestamp = time_to_timestamp(current_time); | ||
| let expiry_secs = last_access_expiry.as_secs(); | ||
| let count = txn.execute( | ||
| "DELETE FROM media WHERE ignore_policy IS FALSE AND (? - last_access) >= ?", |
There was a problem hiding this comment.
The last_access_expiry contains the maximum age of the content. last_access is the timestamp of the last time that the content was accessed. So to get the age of the content we need to do (current_timestamp - last_access).
| /// since Unix Epoch. | ||
| /// | ||
| /// Returns an `i64` as it is the numeric type used by SQLite. | ||
| pub(crate) fn time_to_timestamp(time: SystemTime) -> i64 { |
There was a problem hiding this comment.
Maybe I go too far but… can we have a small tiny unit test for time_to_timestamp please?
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Supercedes #3918. Since I needed to rework everything, I used a new branch.
I believe that I addressed all the comments on #3918. The API around
MediaServicewas not clear to me so I came up with something that works.It is now split into several commits that can be reviewed individually, in order.