Create Timeline struct to hold event chunks from /sync and /messages #230
Conversation
There was a problem hiding this comment.
This looks sensible, though it handles only the simple case when we have chunks that neatly connect with our slices. The more complicated case should be quite common, e.g. we have a normal sync, then we have a sync with a limited timeline.
Now the user requests messages from us that include the second sync, the gap and overlaps into the first sync. We'll need a way to store the gap and merge the slices, well ok, merging is probably optional but we'll certainly want to fill the gap neatly.
We can take some inspiration from hydrogen, also we'll need to be mindful of this fact, which is linked from the hydrogen code.
Also the BTreeMap that stores the indexes would need to be size limited for the memory store, while it would need to be a sled Tree in the sled store. I think you were right that this can be used as the Changes equivalent for timelines and we pass that data to the stores when we write stuff.
matrix_sdk_base/src/store/mod.rs
Outdated
|
|
||
| self.update_events_map_forward(content, last_event_index, next_idx); | ||
| } else if self.is_empty() { | ||
| let next_idx = SliceIdx(u128::MAX / 2); |
There was a problem hiding this comment.
Let's move this into a constructor.
| } | ||
| } | ||
|
|
||
| // TODO: do this in ruma? |
There was a problem hiding this comment.
Yeah, I think this belongs in Ruma.
matrix_sdk_base/src/store/mod.rs
Outdated
| let EventIdx(idx) = e; | ||
| *idx | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
This will default to 0, no? Don't we want to use u128::MAX / 2 here as well?
| // like if there is no prev/next_batch token? | ||
| } | ||
| (None, Some(end)) => {} | ||
| (None, None) => todo!("problems"), |
There was a problem hiding this comment.
I think this happens if you have the whole timeline here? You can't move forward nor backwards, though I think Synapse instead sets the tokens to be the same.
|
|
||
| self.update_events_map_forward(content, u128::MAX / 2, next_idx); | ||
| } else { | ||
| todo!("hmmm we got a problem or a gap") |
There was a problem hiding this comment.
I don't think this means we have a gap? We have a gap if the timeline is marked as limited, so you probably will need to forward that info to this method as well.
| // We have the full chunk already | ||
| (Some(_), Some(_)) => {} | ||
| (Some(_), None) => { | ||
| // We have a gap |
There was a problem hiding this comment.
Is this true? Won't this happen for overlapping stuff as well?
For example
prev_batch end old.start
| | |
A - B - C - D - E - F - G - H - I - J - K
|
|
||
| self.update_events_map_forward( | ||
| // TODO: | ||
| // make the methods more general or specific or |
There was a problem hiding this comment.
We would probably like to deserialize into some event shim that only contains relevant data, e.g. the event id. Why is this needed here, but not in the backward case?
|
Ok so I think what I'm mostly stuck on is what happens when we have to fix the ordering We can't efficiently adjust all the struct SliceIdx(Vec<u8>); // this would allow us to insert chunks by pushing 1'sThis is a problem right? I didn't realize it was quite so common to get overlapping and segmented chunks that could be shifted about at any time really right? |
Seems that I forgot to answer here, I took a look and it seems that Hydrogen just reorders stuff when it needs to merge overlapping stuff. Wouldn't that work? |
Would that mean re-ordering the items in the sled store or somehow keeping track of it and (re)ordering when they are requested? |
I think it means just reordering the slice/event index trees, e.g. events from one slice move into another in a certain order. |
|
Closing in favor of #486 . |
I'm opening a new PR so I can refer to the old one, this approach of starting with the
Timelinestruct is different enough it makes sense to me.We need to keep the
Timelinestruct around from sync/messages to sync/messages we need it to be complete all the time every time. I'm somewhat assuming that the sled store will be the fallback so if theTimelinedoesn't have some info it asks the store?I added a
next_tokenslice map soBTreeMap<NextBatchToken, SliceIdx>. We needed a way of saying I have an incoming /sync with aprev_batchtoken get theSliceIdxwith the previousnext_batchtoken (basically I see no way of not tracking both tokens, other than linear search every time?).It's really early but better to know what I'm doing wrong now. A look over the
handle_messages_responsemethod would be helpful are my assumptions about missing prev/next_batch tokens right?May resolve #138