sliding sync: infer the storage key from the loop id and user id #2008

bnjbvr · 2023-06-02T15:51:50Z

This replaces the storage_key public API with another one enable_caching that doesn't take a string argument. Instead, sliding sync constructors are expected to receive a loop_id argument, that will be used to 1. be part of the storage key for this sliding sync instance, 2. be used as the conn_id (connection id) in the request to the proxy (paving the way for #1928).

The storage key is also inferred from the user id, such that a single app may be used for multiple users.

Fixes #2007.

Signed-off-by: Benjamin Bouvier <public@benj.me>

Hywan

First, I would not name this loop_id but maybe just sync_id. The user of SlidingSync doesn't need to know about “loop” and it can be really confusing for a first discovering of the API.

Second, I don't see the point of this. I take on me the fact I'm probably tired and that I don't understand your long-term plan, but why is it needed to specify the “storage key” directly in the SS constructor?

Hywan · 2023-06-02T18:13:44Z

crates/matrix-sdk/src/sliding_sync/cache.rs

 use tracing::{trace, warn};

 use super::{FrozenSlidingSync, FrozenSlidingSyncList, SlidingSync, SlidingSyncList};
 use crate::{sliding_sync::SlidingSyncListCachePolicy, Client, Result};

+pub(super) fn format_base_storage_key_for_sliding_sync(loop_id: &str, user_id: &UserId) -> String {
+    format!("{}::{}", loop_id, user_id.to_string())


I would prefix with matrix_sdk::sliding_sync:: or something like that to avoid possible naming conflicts.

Done; I've also rejiggered how the other keys were computed. As a note: that means we're changing them, and this will invalidate the cache for all the current users.

bnjbvr · 2023-06-05T07:45:43Z

First, I would not name this loop_id but maybe just sync_id. The user of SlidingSync doesn't need to know about “loop” and it can be really confusing for a first discovering of the API.

That makes sense!

Second, I don't see the point of this. I take on me the fact I'm probably tired and that I don't understand your long-term plan, but why is it needed to specify the “storage key” directly in the SS constructor?

Oh the goal is two-fold here, I am feeding two birds with one seed:

currently if you want to enable caching, you need both to call storage_key() first on a builder, and the presence of that storage key will try to reload the SS instance from the cache. Then the storage_key needs to be propagated to the SS list builders. With this PR, we have a way to infer the storage key, and a very explicit API enable_caching() that tells whether or not we want to use caching in the first place (it's not conditioned to the presence of another dependent data field).
re: two-sync-loops, we'll need a connection id anyways, and we can either automatically provide one (dangerous), or just infer it from somewhere else. That's what I'm proposing here, we can use the sync_id and transform it into a conn_id parameter for the SS request.

Does it make more sense?

Signed-off-by: Benjamin Bouvier <public@benj.me>

Now the prefix is visible only in the `format_storage_key_prefix` function, and other `format_storage_key` function will be based off that. Signed-off-by: Benjamin Bouvier <public@benj.me>

…nformation Signed-off-by: Benjamin Bouvier <public@benj.me>

Hywan

It looks good to me, great job!
Is it possible to enable the caching if cached_list is called? Thoughts?

Hywan · 2023-06-05T08:58:51Z

crates/matrix-sdk/src/sliding_sync/mod.rs

@@ -414,6 +419,7 @@ impl SlidingSync {
            (
                // Build the request itself.
                assign!(v4::Request::new(), {
+                    // conn_id: self.inner.loop_id.clone(),


Is it a todo?

As you remind me, the MSC says that the conn_id must have a max length of 16.

Yeah, a todo that will make sense when we have the dual sync loop split. I've also taken care of validating the identifier in a new version of this PR.

Hywan · 2023-06-05T09:00:02Z

crates/matrix-sdk/src/sliding_sync/client.rs

@@ -7,8 +7,8 @@ use crate::{Client, Result};

 impl Client {
    /// Create a [`SlidingSyncBuilder`] tied to this client.
-    pub fn sliding_sync(&self) -> SlidingSyncBuilder {
-        SlidingSync::builder(self.clone())
+    pub fn sliding_sync(&self, loop_id: impl Into<String>) -> SlidingSyncBuilder {


Suggested change

pub fn sliding_sync(&self, loop_id: impl Into<String>) -> SlidingSyncBuilder {

pub fn sliding_sync<I>(&self, loop_id: I) -> SlidingSyncBuilder where I: Into<String>{

I'm happy to do the change, is there a list of recommended idioms? (I kind of prefer the impl in argument position, as it's slightly faster to type, but I get that it may hide the fact that this function's generic.)

bnjbvr · 2023-06-05T13:50:06Z

Is it possible to enable the caching if cached_list is called? Thoughts?

I've given it a bit more thought, and I think we shouldn't: enable_caching() also enables the caching of the SlidingSync instance itself (to-device token + delta token), and enabling caching for a single list causing that is a bit too implicit, for my taste. But that's easy to add, now or in the future, of course.

Signed-off-by: Benjamin Bouvier <public@benj.me>

bye bye clean commit history Signed-off-by: Benjamin Bouvier <public@benj.me>

codecov · 2023-06-05T14:46:20Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (c6dae67) 74.72% compared to head (28702f1) 74.72%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2008   +/-   ##
=======================================
  Coverage   74.72%   74.72%           
=======================================
  Files         138      138           
  Lines       14952    14952           
=======================================
  Hits        11173    11173           
  Misses       3779     3779

Impacted Files	Coverage Δ
crates/matrix-sdk/src/sliding_sync/mod.rs	`0.00% <ø> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

sliding sync: infer the storage key from the loop id and user id

9344eca

Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr requested a review from a team as a code owner June 2, 2023 15:51

bnjbvr requested review from poljar and removed request for a team June 2, 2023 15:51

Hywan requested changes Jun 2, 2023

View reviewed changes

bnjbvr added 4 commits June 5, 2023 12:14

chore: rename sync_id to id

97c9225

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: check that the sliding sync id is less than 16 chars

f630f5c

Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: rejigger the storage key creation logic

58d969b

Now the prefix is visible only in the `format_storage_key_prefix` function, and other `format_storage_key` function will be based off that. Signed-off-by: Benjamin Bouvier <public@benj.me>

chore: update sliding sync README with API updates and fix outdated i…

7558e93

…nformation Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr requested a review from Hywan June 5, 2023 10:48

Hywan reviewed Jun 5, 2023

View reviewed changes

Merge branch 'main' into infer-storage-key-from-loop-id

7699bf8

Signed-off-by: Benjamin Bouvier <public@benj.me>

Hywan approved these changes Jun 5, 2023

View reviewed changes

chore: clippy + fix test

28702f1

bye bye clean commit history Signed-off-by: Benjamin Bouvier <public@benj.me>

bnjbvr enabled auto-merge (squash) June 5, 2023 14:14

bnjbvr merged commit 61c3a2a into matrix-org:main Jun 5, 2023

bnjbvr deleted the infer-storage-key-from-loop-id branch June 5, 2023 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sliding sync: infer the storage key from the loop id and user id #2008

sliding sync: infer the storage key from the loop id and user id #2008

bnjbvr commented Jun 2, 2023

Hywan left a comment

Hywan Jun 2, 2023

bnjbvr Jun 5, 2023

bnjbvr commented Jun 5, 2023 •

edited

Loading

Hywan left a comment

Hywan Jun 5, 2023

Hywan Jun 5, 2023

bnjbvr Jun 5, 2023 •

edited

Loading

Hywan Jun 5, 2023

bnjbvr Jun 5, 2023

bnjbvr commented Jun 5, 2023

codecov bot commented Jun 5, 2023

	pub fn sliding_sync(&self, loop_id: impl Into<String>) -> SlidingSyncBuilder {
	pub fn sliding_sync<I>(&self, loop_id: I) -> SlidingSyncBuilder where I: Into<String>{

sliding sync: infer the storage key from the loop id and user id #2008

sliding sync: infer the storage key from the loop id and user id #2008

Conversation

bnjbvr commented Jun 2, 2023

Hywan left a comment

Choose a reason for hiding this comment

Hywan Jun 2, 2023

Choose a reason for hiding this comment

bnjbvr Jun 5, 2023

Choose a reason for hiding this comment

bnjbvr commented Jun 5, 2023 • edited Loading

Hywan left a comment

Choose a reason for hiding this comment

Hywan Jun 5, 2023

Choose a reason for hiding this comment

Hywan Jun 5, 2023

Choose a reason for hiding this comment

bnjbvr Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Hywan Jun 5, 2023

Choose a reason for hiding this comment

bnjbvr Jun 5, 2023

Choose a reason for hiding this comment

bnjbvr commented Jun 5, 2023

codecov bot commented Jun 5, 2023

Codecov Report

bnjbvr commented Jun 5, 2023 •

edited

Loading

bnjbvr Jun 5, 2023 •

edited

Loading