-
Notifications
You must be signed in to change notification settings - Fork 289
event cache: add support for running back-pagination #3195
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
Conversation
6e10a9f
to
3c59d3b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3195 +/- ##
==========================================
+ Coverage 83.72% 83.75% +0.03%
==========================================
Files 234 234
Lines 24085 24174 +89
==========================================
+ Hits 20165 20248 +83
- Misses 3920 3926 +6 ☔ View full report in Codecov by Sentry. |
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.
The general idea looks good, and the code looks good.
I'm not keen on the sleeps in unit tests, but don't have better suggestions.
I don't think I have enough context on whether the locking mechanism is good.
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.
I appreciate the effort for the documentation and for the tests. This is still a “WIP” (no offense) as this code will change a lot once #3166 is merged. So far, this is good enough. Thanks for having worked on it!
/// | ||
/// Optionally, wait at most for the given duration for a back-pagination | ||
/// token to be returned by a sync. | ||
pub async fn earliest_backpagination_token( |
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.
Naming proposal: most_recent_backpagination_token
?
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 not the most recent, it's actually the "oldest" 🥲 aka the closest to the start of the timeline. Should I rename it to oldest_backpagination_token
, or even furthest_backpagination_token
?
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.
Oh, then oldest_backpagination_token
makes more sense to me!
@@ -388,16 +450,29 @@ struct RoomEventCacheInner { | |||
/// See comment there. | |||
store: Arc<Mutex<Arc<dyn EventCacheStore>>>, | |||
|
|||
/// A notifier that we received a new pagination token. | |||
pagination_token_notifier: Notify, |
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.
I wonder if we should not have a type to hold pagination data instead of 2 fields in this type.
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.
I think it's a bit premature at this point, but I'll keep this in mind 👍
pub struct PaginationToken(pub String); | ||
|
||
#[derive(Clone)] | ||
pub enum TimelineEntry { |
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.
This type is going to be removed once #3166 is merged: we will need to reconciliate those.
(We can sleep when we're dead.) Also, we can tweak the assertion so it's always valid, depending on the racy behavior. Bleh, but it's better than using sleep.
09d2f3d
to
91f8e74
Compare
This adds support for back-pagination into the event cache, supporting enough features for integrating with the timeline (which is going to happen in a separate PR).
The idea is to provide two new primitives:
The timeline code can then use those two primitives in a loop to replicate the current behavior it has (next PR to be open Soon™).
The representation of events in the store is changed, so that a timeline can have entries, which are one of two things:
This allows us to avoid a lot of complexity from the back-pagination code in the timeline, where we'd attach the backpagination token to an event that only had an event_id. We don't have to do this here, and I suppose we could even attach the backpagination token to the next event itself.
This doesn't do reconciliation yet; the plan is to add it as a next step.