Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Replace EventContext fields prev_group and delta_ids with field state_group_deltas #15233

Merged
merged 28 commits into from
Jun 13, 2023

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Mar 9, 2023

Currently the class EventContext uses the fields prev_group and delta_ids to hold information about the state group previous to the event (or the state group previous to the group before the event) and the state delta between the previous state group and the new state group. This information was used to try and optimize storing the state at the event, as if the previous state group was known one could just store the state delta between the old state and the new.

However, this way of tracking the information could be very confusing. This PR attempts to make it less confusing, and replaces the fields prev_group and delta_ids with the field state_group_deltas in the hope that this formulation is a little clearer and easier to work with.

@H-Shay H-Shay requested a review from a team as a code owner March 9, 2023 03:23
@H-Shay H-Shay self-assigned this Mar 9, 2023
@H-Shay H-Shay marked this pull request as draft March 9, 2023 05:35
@H-Shay H-Shay removed the request for review from a team March 9, 2023 05:35
@H-Shay H-Shay marked this pull request as ready for review March 9, 2023 23:12
@H-Shay H-Shay requested a review from a team March 9, 2023 23:12
changelog.d/15233.misc Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
@clokep clokep self-requested a review March 22, 2023 11:04
@clokep
Copy link
Member

clokep commented Apr 10, 2023

@H-Shay Is this pending my review? I think you had said in a meeting recently that you were going to look at this again, but want to ensure that I'm not blocking you!

@clokep clokep removed their request for review May 1, 2023 15:30
@clokep
Copy link
Member

clokep commented May 1, 2023

We talked about this in person and @H-Shay is going to look into whether some of this code is even truly used in "real" situations or if the optimizations aren't needed. Once we figure that out we'll decide what to do with this PR.

Removing my review request for now.

@H-Shay
Copy link
Contributor Author

H-Shay commented May 16, 2023

Right so I think this is ready for another look.

For completeness, Erik and I logged how often the optimization that prev_group/state_group_deltas enables was hit and determined it was worth keeping. Thus I have attempted here to rework the PR to make the code clearer by reducing the amount of Nones flying around, better documentation, more documentation closer to the relevant code, and some encapsulation. Let me know what you think.

@H-Shay H-Shay requested a review from clokep May 16, 2023 18:53
Copy link
Member

@clokep clokep 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 this is massively clearer without all the Nones floating around! I think most of my comments are minor refactoring.

tests/test_state.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
H-Shay and others added 3 commits May 17, 2023 10:37
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

It seems I never hit submit on this review. 😢 I think you've already updated a few of these things...

synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
synapse/events/snapshot.py Outdated Show resolved Hide resolved
docs/upgrade.md Outdated Show resolved Hide resolved
docs/upgrade.md Outdated Show resolved Hide resolved
@erikjohnston
Copy link
Member

I think this is back on @H-Shay's plate?

@erikjohnston erikjohnston removed their request for review June 8, 2023 12:06
@H-Shay
Copy link
Contributor Author

H-Shay commented Jun 12, 2023

Sooo I think I've sorted the backwards/forwards compatibility issue: serializing produces a dict which de-serializing then converts back into an object.
The modes of possible incompatibility then are:

  1. The event context is serialized by old code and de-serialized by new code: to address that the new de-serialization code checks for the existence of the key "state_group_deltas" in the dict and if it doesn't exist just uses an empty dict for that field in the object it is building and ignores the presence of the old keys in the dict
  2. The event context is serialized by the new code and de-serialized by the old code: to address this I had the new code add two "dummy" keys set to None corresponding to the old fields we removed ("prev_group" and "delta_ids") so the old de-serialize code will use those to reconstruct the object and ignore the new key "state_group_deltas" in the dict.

I think this solves the problem? Let me know if my logic seems wacky. (We might want to also add in this case that there may be a slight performance penalty until all workers and master are on > v1.87.0)

@H-Shay H-Shay requested a review from erikjohnston June 12, 2023 18:55
@erikjohnston
Copy link
Member

I think this solves the problem? Let me know if my logic seems wacky.

Sounds correct to me!

(We might want to also add in this case that there may be a slight performance penalty until all workers and master are on > v1.87.0)

TBH I wouldn't bother

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Woo! 🎉

We should add a future maintenance task to remove the backwards compat hack in a couple of releases.

@H-Shay H-Shay merged commit 553f2f5 into develop Jun 13, 2023
@H-Shay H-Shay deleted the shay/kill_prev_with_fire branch June 13, 2023 20:22
@H-Shay
Copy link
Contributor Author

H-Shay commented Jun 13, 2023

Merged #15233 into develop, thanks for the reviews.

#15777 is the tracking issue for removing the backwards compat code in the future.

yingziwu added a commit to yingziwu/synapse that referenced this pull request Aug 21, 2023
Please note that this will be the last release of Synapse that is compatible with
Python 3.7 and earlier.
This is due to Python 3.7 now having reached End of Life; see our [deprecation policy](https://matrix-org.github.io/synapse/v1.87/deprecation_policy.html)
for more details.

- Pin `pydantic` to `^1.7.4` to avoid backwards-incompatible API changes from the 2.0.0 release.
  Resolves matrix-org#15858.
  Contributed by @PaarthShah. ([\matrix-org#15862](matrix-org#15862))

- Split out 2022 changes from the changelog so the rendered version in GitHub doesn't timeout as much. ([\matrix-org#15846](matrix-org#15846))

- Improve `/messages` response time by avoiding backfill when we already have messages to return. ([\matrix-org#15737](matrix-org#15737))
- Add spam checker module API for logins. ([\matrix-org#15838](matrix-org#15838))

- Fix a long-standing bug where media files were served in an unsafe manner. Contributed by @joshqou. ([\matrix-org#15680](matrix-org#15680))
- Avoid invalidating a cache that was just prefilled. ([\matrix-org#15758](matrix-org#15758))
- Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15770](matrix-org#15770))
- Fix joining rooms through aliases where the alias server isn't a real homeserver. Contributed by @tulir @ Beeper. ([\matrix-org#15776](matrix-org#15776))
- Fix a bug in push rules handling leading to an invalid (per spec) `is_user_mention` rule sent to clients. Also fix wrong rule names for `is_user_mention` and `is_room_mention`. ([\matrix-org#15781](matrix-org#15781))
- Fix a bug introduced in 1.57.0 where the wrong table would be locked on updating database rows when using SQLite as the database backend. ([\matrix-org#15788](matrix-org#15788))
- Fix Sytest environmental variable evaluation in CI. ([\matrix-org#15804](matrix-org#15804))
- Fix forgotten rooms missing from initial sync after rejoining them. Contributed by Nico from Famedly. ([\matrix-org#15815](matrix-org#15815))
- Fix sqlite `user_filters` upgrade introduced in v1.86.0. ([\matrix-org#15817](matrix-org#15817))

- Document `looping_call()` functionality that will wait for the given function to finish before scheduling another. ([\matrix-org#15772](matrix-org#15772))
- Fix a typo in the [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html). ([\matrix-org#15805](matrix-org#15805))
- Fix typo in MSC number in faster remote room join architecture doc. ([\matrix-org#15812](matrix-org#15812))

- Remove experimental [MSC2716](matrix-org/matrix-spec-proposals#2716) implementation to incrementally import history into existing rooms. ([\matrix-org#15748](matrix-org#15748))

- Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas`. ([\matrix-org#15233](matrix-org#15233))
- Regularly try to send transactions to other servers after they failed instead of waiting for a new event to be available before trying. ([\matrix-org#15743](matrix-org#15743))
- Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15755](matrix-org#15755))
- Allow for the configuration of max request retries and min/max retry delays in the matrix federation client. ([\matrix-org#15783](matrix-org#15783))
- Switch from `matrix://` to `matrix-federation://` scheme for internal Synapse routing of outbound federation traffic. ([\matrix-org#15806](matrix-org#15806))
- Fix harmless exceptions being printed when running the port DB script. ([\matrix-org#15814](matrix-org#15814))

* Bump attrs from 22.2.0 to 23.1.0. ([\matrix-org#15801](matrix-org#15801))
* Bump cryptography from 40.0.2 to 41.0.1. ([\matrix-org#15800](matrix-org#15800))
* Bump ijson from 3.2.0.post0 to 3.2.1. ([\matrix-org#15802](matrix-org#15802))
* Bump phonenumbers from 8.13.13 to 8.13.14. ([\matrix-org#15798](matrix-org#15798))
* Bump ruff from 0.0.265 to 0.0.272. ([\matrix-org#15799](matrix-org#15799))
* Bump ruff from 0.0.272 to 0.0.275. ([\matrix-org#15833](matrix-org#15833))
* Bump serde_json from 1.0.96 to 1.0.97. ([\matrix-org#15797](matrix-org#15797))
* Bump serde_json from 1.0.97 to 1.0.99. ([\matrix-org#15832](matrix-org#15832))
* Bump towncrier from 22.12.0 to 23.6.0. ([\matrix-org#15831](matrix-org#15831))
* Bump types-opentracing from 2.4.10.4 to 2.4.10.5. ([\matrix-org#15830](matrix-org#15830))
* Bump types-setuptools from 67.8.0.0 to 68.0.0.0. ([\matrix-org#15835](matrix-org#15835))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants