Add two streams to get the timeline of a room and store timeline to SledStore #486

jsparber · 2022-02-08T15:03:42Z

This is the first part of #288 (comment)

This doesn't yet implement a local cache for messages.

To implement the message cache the store implementation needs to collect the TimelineSlices received via the StateChanges struct. The store then needs to return a stream of events via StateStore::room_timeline().

jsparber · 2022-02-08T16:16:23Z

I'm not sure how to fix the issue with the matrix-sdk-exmplae-wasm_command_bot example.

gnunicorn · 2022-02-08T17:18:31Z

@jsparber it appears that the SinkExt you are using is an optional feature of future-util, hidden behind the sink-feature-flag, which we are not activating. I suspect the only reason it works usually is because something else is activating it when we are std. However, the proper fix is to add features = ["sink"] to future-util entry in the Cargo.toml.

jsparber · 2022-02-08T17:24:19Z

@jsparber it appears that the SinkExt you are using is an optional feature of future-util, hidden behind the sink-feature-flag, which we are not activating. I suspect the only reason it works usually is because something else is activating it when we are std. However, the proper fix is to add features = ["sink"] to future-util entry in the Cargo.toml.

i think i tried that already. And it caused it to fail differently. Let's see what happens.

jsparber · 2022-02-10T10:52:21Z

@gnunicorn I figured out that we don't even need SinkExt i should have used unbounded_send

jsparber · 2022-02-11T19:17:55Z

I now implemented also storing events to the statestore for SledStore. I think this is ready for a review and feedback. Even though I still need to add some tests for it. Right now I tested it with https://gitlab.gnome.org/GNOME/fractal/-/merge_requests/985

jsparber · 2022-02-14T17:08:21Z

I added now also tests :)

poljar · 2022-02-15T12:11:31Z

Could you please rebase to get rid of the CI error?

gnunicorn

I have a few API and some implementation request.

crates/matrix-sdk-base/src/rooms/normal.rs

crates/matrix-sdk-base/src/store/sled_store.rs

crates/matrix-sdk-base/src/timeline_stream.rs

crates/matrix-sdk/src/client.rs

crates/matrix-sdk-base/src/timeline_stream.rs

crates/matrix-sdk-common/src/deserialized_responses.rs

crates/matrix-sdk/src/client.rs

crates/matrix-sdk/src/room/common.rs

crates/matrix-sdk-base/src/timeline_stream.rs

crates/matrix-sdk/src/room/common.rs

crates/matrix-sdk-base/src/timeline_stream.rs

The TimelineSlice is a slice of the timeline of a room and contains the start and end of the slice.

This writes slices from StateChanges::timeline to the SledStore. The cache is removed when ever a limited sync response is received or when an event already known to the store is received. Stored events are redacted when a redaction event is received.

The test is only run when the `sled_state_store` feature is enabled. This also changes the test_json::MORE_SYNC and test_json::MORE_SYNC_2 to not responsed with a limited timeline.

gnunicorn · 2022-02-18T10:48:36Z

@jsparber please don't force push on PRs (or any published branches for that matter), it makes tracking of changes hard to impossible. Namely, if the branch has been pulled locally, it messes with git and, more annoyingly, the Github Review features can't keep track of the changes, requiring reviewers to not only review the new changes made but the entire PR as a whole again. Thus making it more (annoying) work, that is more error-prone (because changes are easier overlooked) and delaying the process of getting things reviewed and merged.

jsparber · 2022-02-18T10:52:10Z

@jsparber please don't force push on PRs (or any published branches for that matter), it makes tracking of changes hard to impossible. Namely, if the branch has been pulled locally, it messes with git and, more annoyingly, the Github Review features can't keep track of the changes, requiring reviewers to not only review the new changes made but the entire PR as a whole again. Thus making it more (annoying) work, that is more error-prone (because changes are easier overlooked) and delaying the process of getting things reviewed and merged.

Oh sorry, i'm used to that. I think Gitlab handles rebases better, and we do it all the time to keep the commit history clean and it works fairly well. I won't do it here anymore.

jsparber · 2022-02-18T11:03:24Z

@gnunicorn Do you also don't want me to rebase the branch on top of main?

gnunicorn · 2022-02-18T11:41:30Z

@jsparber just merge main in, this is no problem.

gnunicorn

minor nitpicks! Thanks heaps!

crates/matrix-sdk-base/src/store/indexeddb_store.rs

gnunicorn · 2022-02-22T11:10:59Z

crates/matrix-sdk-base/src/store/indexeddb_store.rs

+                                    if room_version.is_none() {
+                                        room_version = Some(room_infos
+                                                        .get(&room_key)?
+                                                        .await?
+                                                        .map(|r| self.deserialize_event::<RoomInfo>(r))
+                                                        .transpose()?
+                                                        .and_then(|info| {
+                                                            info.base_info
+                                                                .create
+                                                                .map(|event| event.room_version)
+                                                        }).unwrap_or_else(|| {
+                                                            warn!("Unable to find the room version for {}, assume version 9", room_id);
+                                                            RoomVersionId::V9
+                                                        }));
+                                    }
+
+                                    full_event.event = Raw::new(&AnySyncRoomEvent::from(
+                                        inner_event
+                                            .redact(redaction, room_version.as_ref().unwrap()),
+                                    ))?;


If we unwrap the room version next anyways, why store it into a Some first? Instead let's do (still needs formatting fixes, but you get the gist):

Suggested change

if room_version.is_none() {

room_version = Some(room_infos

.get(&room_key)?

.await?

.map(|r| self.deserialize_event::<RoomInfo>(r))

.transpose()?

.and_then(|info| {

info.base_info

.create

.map(|event| event.room_version)

}).unwrap_or_else(|| {

warn!("Unable to find the room version for {}, assume version 9", room_id);

RoomVersionId::V9

}));

}

full_event.event = Raw::new(&AnySyncRoomEvent::from(

inner_event

.redact(redaction, room_version.as_ref().unwrap()),

))?;

let room_v = if let Some(room_v) = room_version {

room_v

} else {

room_infos

.get(&room_key)?

.await?

.map(|r| self.deserialize_event::<RoomInfo>(r))

.transpose()?

.and_then(|info| {

info.base_info

.create

.map(|event| event.room_version)

}).unwrap_or_else(|| {

warn!("Unable to find the room version for {}, assume version 9", room_id);

RoomVersionId::V9

}))

};

}

full_event.event = Raw::new(&AnySyncRoomEvent::from(

inner_event

.redact(redaction, room_v),

))?;

the idea is that we only lookup the room version once per TimelineSlice and not for every single event we need to remove.

we could also use OnceCell but i didn't want to introduce a new crate for this single use case.

Well... but why within the loop though? Am I missing something or isn't everything within that call already available on line 592 and thus should just happen there outside/before the loop? Arguably as it happens in both cases of iteration - even outside of this?

Generally, more than 4 levels of indentation are very hard to read and follow (and makes the lines break more often than necessary), I wonder if we can't just make some of them simpler... and moving the block out would already do that.

Before we check events we can't be sure if we will need the room version, therefore i look it up only at the first use

gnunicorn · 2022-02-22T14:49:18Z

crates/matrix-sdk-base/src/store/memory_store.rs

+                        if let Some(position) = data.event_id_to_position.get(&redaction.redacts) {
+                            if let Some(mut full_event) = data_events.get_mut(position) {
+                                let inner_event = full_event.event.deserialize()?;
+                                if room_version.is_none() {


same as above

crates/matrix-sdk-base/src/store/indexeddb_store.rs

crates/matrix-sdk-base/src/store/mod.rs

gnunicorn · 2022-02-22T14:55:20Z

crates/matrix-sdk-base/src/store/sled_store.rs

+                                .transpose()?
+                            {
+                                let inner_event = full_event.event.deserialize()?;
+                                if room_version.is_none() {


same as above

gnunicorn · 2022-02-22T14:57:35Z

crates/matrix-sdk-base/src/store/sled_store.rs

+            event_id_to_position_batch.remove(key?)
+        }
+
+        let ret: Result<(), TransactionError<SerializationError>> =


why do these have their own transactions and are not bundled together with the other transaction? A transaction failing implies that none of the given data has been submitted, but by splitting this into three functions, if only the last transaction fails, we have an (not terribly) inconsistent database state. Let's move these into the same transaction closure pls

unfortunately this isn't possible, because of a limitation of sledstore. Since this issue isn't introduced by this MR, i would prefer to handle it separately.

unfortunately this isn't possible, because of a limitation of sledstore.

Actually, i think it would be. We can uses batches.

@poljar what do you think?

crates/matrix-sdk/src/client.rs

crates/matrix-sdk/src/room/common.rs

jsparber force-pushed the timeline_stream branch from b12fbc4 to 7ee2515 Compare February 8, 2022 17:23

jsparber force-pushed the timeline_stream branch 3 times, most recently from 81c9939 to a6e8fcf Compare February 10, 2022 10:29

jsparber force-pushed the timeline_stream branch 2 times, most recently from 7a3a71d to 63bc7ab Compare February 11, 2022 19:15

jsparber force-pushed the timeline_stream branch 3 times, most recently from 19af93e to 76a7d79 Compare February 11, 2022 19:50

jsparber changed the title ~~Add two streams to get the timeline of a room~~ Add two streams to get the timeline of a room and store timeline to SledStore Feb 11, 2022

jsparber force-pushed the timeline_stream branch 3 times, most recently from 228df67 to 2d98787 Compare February 14, 2022 14:25

jsparber mentioned this pull request Feb 14, 2022

Use SledStore to store full timeline #288

Closed

jsparber force-pushed the timeline_stream branch 3 times, most recently from 1b86797 to b1d1ae8 Compare February 15, 2022 13:19

gnunicorn suggested changes Feb 16, 2022

View reviewed changes

poljar requested changes Feb 16, 2022

View reviewed changes

jsparber force-pushed the timeline_stream branch from b1d1ae8 to 68239d3 Compare February 17, 2022 15:02

jsparber added 3 commits February 17, 2022 16:13

common: add TimelineSlice

c5950ac

The TimelineSlice is a slice of the timeline of a room and contains the start and end of the slice.

Add stream to obtain the timeline of a room

64c49cd

test: Add mocked responses need for timeline storage

7d19e9a

store: Save timeline to SledStore

516aa26

This writes slices from StateChanges::timeline to the SledStore. The cache is removed when ever a limited sync response is received or when an event already known to the store is received. Stored events are redacted when a redaction event is received.

jsparber force-pushed the timeline_stream branch from 68239d3 to 89fff46 Compare February 17, 2022 15:15

jsparber added 3 commits February 17, 2022 18:32

store: Save timeline to MemoryStore

fb3edeb

Add test for timeline cache

9e7bee8

Add test for matrix_sdk::room::Common::timeline()

8e80e02

The test is only run when the `sled_state_store` feature is enabled. This also changes the test_json::MORE_SYNC and test_json::MORE_SYNC_2 to not responsed with a limited timeline.

jsparber force-pushed the timeline_stream branch from 89fff46 to 8e80e02 Compare February 17, 2022 17:32

jsparber added 6 commits February 22, 2022 11:43

store: Save timeline to IndexedDbStore

0880867

Merge remote-tracking branch 'origin/main' into timeline_stream

d7592cc

Fix example for Room::Common::timeline()

84c46af

Add method to optain timeline stream in one direction

7627c14

store: Use BoxStream type without explicit lifetime

890a867

Fix style issue

70e826f

gnunicorn approved these changes Feb 22, 2022

View reviewed changes

gnunicorn reviewed Feb 22, 2022

View reviewed changes

crates/matrix-sdk/src/room/common.rs Show resolved Hide resolved

jsparber and others added 5 commits February 22, 2022 16:49

store: Add comment why we can ignore unexpected TimelineSlices

3378d18

Merge remote-tracking branch 'origin/main' into timeline_stream

980076f

minor refactor for simplicity

c22b4fc

room version simplification

2598001

Clippy and Cargo fmt

78af96d

gnunicorn requested a review from poljar February 23, 2022 11:58

poljar approved these changes Feb 23, 2022

View reviewed changes

gnunicorn merged commit 8414718 into matrix-org:main Feb 23, 2022

This was referenced Feb 23, 2022

Store the timeline in the SledStore #141

Closed

Create Timeline struct to hold event chunks from /sync and /messages #230

Closed

poljar mentioned this pull request Feb 28, 2022

Deadlock in the timeline handling of the memory store. #508

Closed

poljar mentioned this pull request Apr 25, 2022

Storing timelines in the state store #138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add two streams to get the timeline of a room and store timeline to SledStore #486

Add two streams to get the timeline of a room and store timeline to SledStore #486

jsparber commented Feb 8, 2022

jsparber commented Feb 8, 2022

gnunicorn commented Feb 8, 2022

jsparber commented Feb 8, 2022

jsparber commented Feb 10, 2022

jsparber commented Feb 11, 2022

jsparber commented Feb 14, 2022

poljar commented Feb 15, 2022

gnunicorn left a comment

gnunicorn commented Feb 18, 2022

jsparber commented Feb 18, 2022

jsparber commented Feb 18, 2022

gnunicorn commented Feb 18, 2022

gnunicorn left a comment

gnunicorn Feb 22, 2022

jsparber Feb 22, 2022

jsparber Feb 22, 2022

gnunicorn Feb 23, 2022 •

edited

Loading

jsparber Feb 23, 2022

gnunicorn Feb 22, 2022

gnunicorn Feb 22, 2022

gnunicorn Feb 22, 2022

jsparber Feb 22, 2022

jsparber Feb 22, 2022

Add two streams to get the timeline of a room and store timeline to SledStore #486

Add two streams to get the timeline of a room and store timeline to SledStore #486

Conversation

jsparber commented Feb 8, 2022

jsparber commented Feb 8, 2022

gnunicorn commented Feb 8, 2022

jsparber commented Feb 8, 2022

jsparber commented Feb 10, 2022

jsparber commented Feb 11, 2022

jsparber commented Feb 14, 2022

poljar commented Feb 15, 2022

gnunicorn left a comment

Choose a reason for hiding this comment

gnunicorn commented Feb 18, 2022

jsparber commented Feb 18, 2022

jsparber commented Feb 18, 2022

gnunicorn commented Feb 18, 2022

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnunicorn Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnunicorn Feb 23, 2022 •

edited

Loading