Skip to content
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

feat: Migrate from Sliding Sync to Simplified Sliding Sync #3676

Merged
merged 11 commits into from
Jul 17, 2024

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Jul 10, 2024

We want to support the Simplified MSC3575 (temporary name).

This PR should be reviewed patch-by-patch. It's not super complex, but you need to understand the approach:

  1. Create a matrix_sdk_base::sliding_sync::http module, which provides all the HTTP types for MSC3575,
  2. Migrate the entire codebase to use this module instead of ruma: it's just type aliases at this point, but it's important for the next steps; tl;dr: s/ruma::…::sync::v4/matrix_sdk_base::sliding_sync::http:ss,
  3. This http module then provides HTTP types for Simplified MSC3575 (!) + conversions from MSC3575 to Simplified MSC3575, and exposes Simplfied MSC3575 as the default types,
  4. Switch the entire codebase to use Simplified MSC3575; tl;dr: s/http:ss/http + remove features from MSC3575 that are no longer used in Simplified MSC3575 (like sort, bump_event_types, unsubscribe_rooms) or rename some features (like timestamp to bump_stamp),
  5. Finally, since now the entire codebase uses Simplified MSC3575, we need a mechanism to convert the Simplified MSC3575 Request into a MSC3575 Request and the opposite for the Response. It's done with a experimental-not-simplified-sliding-sync flag.
  6. … Bonus: experimental-not-simplified-sliding-sync turned to be difficult to use esp. when testing the whole project with --workspace. Instead, we went a bit further and replace this compiler feature flag by a runtime configuration on the Client with Client::is_simplified_sliding_sync_enabled. That way we can decide at runtime if we want Simplified MSC3575 or the ol' MSC3575.

Why this approach?

  • The day we fully sunset MSC3575, we simply remove Client::is_simplified_sliding_sync_enabled
  • Since we can decide at runtime to enable Simplified or regular MSC3575, it's easier for the apps (like Element X, Fractal etc.) to play with it.

@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch 2 times, most recently from 7260887 to f82dc72 Compare July 10, 2024 14:23
@Hywan Hywan mentioned this pull request Jul 10, 2024
32 tasks
@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch from f82dc72 to c7979cb Compare July 10, 2024 15:37
@Hywan Hywan marked this pull request as ready for review July 10, 2024 15:38
@Hywan Hywan requested a review from a team as a code owner July 10, 2024 15:38
@Hywan Hywan requested review from bnjbvr and removed request for a team July 10, 2024 15:38
@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch from afbecf0 to 6181db3 Compare July 11, 2024 12:12
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please request another round of review when the PR is stable and there are no new commits while I'm reviewing :)

@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch 4 times, most recently from 5fb806d to 1b5ecaa Compare July 12, 2024 11:35
@Hywan Hywan requested a review from bnjbvr July 12, 2024 11:39
@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch from 6cf2ec1 to 0bd5ec3 Compare July 13, 2024 07:34
Copy link

codecov bot commented Jul 13, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 84.01%. Comparing base (b79fdef) to head (07b2004).

Files Patch % Lines
crates/matrix-sdk/src/sliding_sync/builder.rs 47.05% 9 Missing ⚠️
crates/matrix-sdk/src/sliding_sync/mod.rs 80.00% 4 Missing ⚠️
crates/matrix-sdk-base/src/sliding_sync/mod.rs 85.71% 1 Missing ⚠️
...s/matrix-sdk-ui/src/room_list_service/room_list.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3676      +/-   ##
==========================================
- Coverage   84.37%   84.01%   -0.37%     
==========================================
  Files         260      260              
  Lines       26783    26700      -83     
==========================================
- Hits        22598    22431     -167     
- Misses       4185     4269      +84     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr
bnjbvr previously requested changes Jul 15, 2024
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool to see SSS coming to life, I appreciate the amount of work on this and done in Ruma under the hood 👏

There are a few strong no-noes in that PR that make it impossible to land in the current shape.

9961db5 should be in Ruma, not in our code. Too many code smells, likely too hard to maintain in the long term.

a9f22d1 was super hard to review. From a glance, it would have seemed much much simpler to review if it were split into smaller commits:

  • get rid of the delta token (to fuse with the latest commit)
  • get rid of the unsubscribe mechanism (why can we do that? we still have to support SS as it is right now)
  • remove the ::ss namespacing in most of the code
  • rename the recency_timestamp to recency_stamp and associated code

I understand that all these changes must come before the actual migration from SS to SSS, but it would've been much much simpler to review if those changes were smaller (usually when i see a commit titled "rename X to Y", if i agree with the renaming i'll skim the code real-quick, and that takes me much less time, than trying to disentangle many changes that live in the same commit).

Pretty sure there's a big regression for SS, since we get rid of the unsubscribe() mechanism a bit too early in the patch set.

Also: do we need a8a1315? I thought we said that having static configuration was sufficient…

crates/matrix-sdk-base/src/lib.rs Show resolved Hide resolved
crates/matrix-sdk-base/src/sliding_sync/http.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/sliding_sync/README.md Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/sliding_sync/http.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/sliding_sync/http.rs Outdated Show resolved Hide resolved
bindings/matrix-sdk-ffi/Cargo.toml Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/sliding_sync/http.rs Outdated Show resolved Hide resolved
testing/matrix-sdk-integration-testing/src/helpers.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/sliding_sync/http.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/sliding_sync/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this looks fine, the PR is a bit chunky but most of it are type swaps.

I left some small nits, feel free to merge once addressed. Bonus points if you get the history cleaned up as well.

Cargo.toml Outdated Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/normal.rs Show resolved Hide resolved
crates/matrix-sdk-base/src/rooms/normal.rs Show resolved Hide resolved
crates/matrix-sdk-base/src/sliding_sync/http.rs Outdated Show resolved Hide resolved
testing/matrix-sdk-integration-testing/Cargo.toml Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/builder.rs Show resolved Hide resolved
crates/matrix-sdk/src/sliding_sync/cache.rs Show resolved Hide resolved
@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch from da7e129 to 00b6bbc Compare July 17, 2024 13:36
Hywan added 7 commits July 17, 2024 15:41
…eld.

This patch is the first over two patches to change the support
of Simplified MSC3575 from a compiler feature flag to a runtime
configuration. This configuration is held by the `Client`.
By default, it's turned off inside `matrix-sdk-ffi` and
`matrix-sdk-integration-testing`, otherwise it's turned on.
This patch moves the `sliding_sync` file into its own module.
This patch creates an `http` module containing all the sliding sync types
types (from Simplified MSC3575 or simply MSC3575).
…nc_once`.

This patch extracts most of `SlidingSync::sync_once` into a
method named `SlidingSync::send_sync_request`. The name mimics
the `SlidingSync::generate_sync_request` similar method: first we
_generate_, then we _send_.

The `SlidingSync::send_sync_request` is generic over the `Request` and
`::sync_once` passes the correct type depending of whether Simplified
MSC3575 is enabled.
Simplified sliding sync no longer has the concept of unsubscribing from
a room. This patch removes this API.
This patch renames “recency timestamp” to “recency stamp”. It prepares
the fact that simplified sliding sync has a `bump_stamp` instead of a
`timestamp`. The notion of _timestamp_ must be removed.
Simplified sliding sync no longer has the `is_tombstoned` filter. This
patch removes it.
Hywan added 2 commits July 17, 2024 15:48
Simplified sliding sync no longer has `sort` or `bump_event_types`
because they are static values on the implementation/server side. This
patch removes them here.
Simplified sliding sync doesn't have `delta_token`. This patch removes
it. Note: even the current sliding sync proxy doesn't use it.
@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch from 00b6bbc to 34bcd6d Compare July 17, 2024 14:13
Hywan added 2 commits July 17, 2024 16:16
…g sync.

This patch migrates the entire SDK to sliding sync to simplified sliding
sync.
@Hywan Hywan force-pushed the feat-sdk-sliding-sync-types branch from 34bcd6d to 07b2004 Compare July 17, 2024 14:16
@bnjbvr bnjbvr dismissed their stale review July 17, 2024 14:29

damir reviewed it

@Hywan Hywan merged commit dd20c37 into matrix-org:main Jul 17, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants