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

Message retention policies at the room and server levels #5815

Merged
merged 2 commits into from Aug 28, 2019

Conversation

@babolivier
Copy link
Member

commented Aug 2, 2019

Implements most of MSC1763, except for the management of min_period in a room's retention policy.

This is not a generic implementation, because it relies on server admins setting boundaries for what users can provide as values in a room's retention policy, and considers the rooms lacking a policy as following the server's. Changing that should be fairly trivial, though, and without consequences.

Note to the reviewer: Sorry this PR is a bit large, every commit should be reviewable independently.

@babolivier babolivier added the dinsic label Aug 2, 2019
@codecov

This comment has been minimized.

Copy link

commented Aug 2, 2019

Codecov Report

Merging #5815 into dinsic will decrease coverage by 18.46%.
The diff coverage is 20.43%.

@@             Coverage Diff             @@
##           dinsic    #5815       +/-   ##
===========================================
- Coverage    63.6%   45.13%   -18.47%     
===========================================
  Files         332      332               
  Lines       36459    36543       +84     
  Branches     6018     6035       +17     
===========================================
- Hits        23191    16495     -6696     
- Misses      11635    19034     +7399     
+ Partials     1633     1014      -619
@codecov

This comment has been minimized.

Copy link

commented Aug 2, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (dinsic@62f5e3b). Click here to learn what that means.
The diff coverage is 61.65%.

@@            Coverage Diff            @@
##             dinsic    #5815   +/-   ##
=========================================
  Coverage          ?   63.74%           
=========================================
  Files             ?      332           
  Lines             ?    36691           
  Branches          ?     6085           
=========================================
  Hits              ?    23388           
  Misses            ?    11648           
  Partials          ?     1655
@babolivier

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

FTR I haven't got the time to include tests yet, and am planning on writing some on Monday. In the mean time, what's already here should be functional and reviewable.

@babolivier babolivier marked this pull request as ready for review Aug 16, 2019
@babolivier babolivier requested a review from matrix-org/synapse-core Aug 16, 2019
Copy link
Member

left a comment

I haven't dug right into the the code, but I'm a bit confused about what the settings are meant to do and how they are meant to interact. I think maybe we need to reconsider how this works?

synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/handlers/pagination.py Outdated Show resolved Hide resolved
@richvdh

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Also: Please make sure that you test that backfill behaves sensibly once a purge has happened

@babolivier babolivier requested a review from matrix-org/synapse-core Aug 21, 2019
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/storage/room.py Outdated Show resolved Hide resolved
synapse/storage/room.py Outdated Show resolved Hide resolved
synapse/storage/room.py Outdated Show resolved Hide resolved
synapse/storage/room.py Outdated Show resolved Hide resolved
synapse/storage/room.py Outdated Show resolved Hide resolved
@babolivier

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

For the record, after some discussion with @erikjohnston, we figured it would be better and less risky to hide expired events from the clients instead of filtering out at the federation level, and to just let the expired events get deleted by the purge job(s). @giomfo approved this change on behalf of DINSIC.

The current state of this PR should reflect this decision.

@babolivier babolivier requested a review from matrix-org/synapse-core Aug 27, 2019
Copy link
Member

left a comment

This is looking much better! A few things, but I think they're all pretty small.

synapse/config/server.py Show resolved Hide resolved
synapse/federation/federation_server.py Outdated Show resolved Hide resolved
synapse/handlers/federation.py Outdated Show resolved Hide resolved
synapse/handlers/pagination.py Outdated Show resolved Hide resolved
synapse/handlers/pagination.py Outdated Show resolved Hide resolved
synapse/storage/room.py Show resolved Hide resolved
synapse/storage/room.py Outdated Show resolved Hide resolved
synapse/storage/schema/delta/55/room_retention.sql Outdated Show resolved Hide resolved
synapse/visibility.py Outdated Show resolved Hide resolved
synapse/visibility.py Outdated Show resolved Hide resolved
@babolivier

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

In e200257 I added a new event_id column to the retention table because @erikjohnston pointed out that _update_retention_policy_for_room_txn would also be called on events received by backfilling, therefore there was a risk for an old event to overwrite more recent data and to end up with a room_retention table that doesn't represent the current state of rooms.

With this new column, we can do an INNER JOIN with the current_state_events table to figure out which policy is the right one to apply.

@babolivier babolivier requested a review from matrix-org/synapse-core Aug 28, 2019
synapse/storage/room.py Outdated Show resolved Hide resolved
synapse/storage/room.py Outdated Show resolved Hide resolved
synapse/handlers/pagination.py Outdated Show resolved Hide resolved
@babolivier babolivier requested a review from matrix-org/synapse-core Aug 28, 2019
Copy link
Member

left a comment

🚢

@babolivier babolivier force-pushed the babolivier/dinsic-message-retention branch 2 times, most recently from 077609b to c8a113a Aug 28, 2019
babolivier added 2 commits Jul 24, 2019
@babolivier babolivier force-pushed the babolivier/dinsic-message-retention branch from c8a113a to e5df12a Aug 28, 2019
@babolivier babolivier merged commit 99eec6d into dinsic Aug 28, 2019
21 checks passed
21 checks passed
buildkite/synapse Build #3784 passed (26 minutes, 59 seconds)
Details
buildkite/synapse/check-sample-config Passed (59 seconds)
Details
buildkite/synapse/isort Passed (19 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (17 seconds)
Details
buildkite/synapse/packaging Passed (21 seconds)
Details
buildkite/synapse/pep-8 Passed (43 seconds)
Details
buildkite/synapse/pipeline Passed (8 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-postgres-9-dot-4 Passed (24 minutes, 50 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-postgres-9-dot-5 Passed (24 minutes, 56 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-sqlite Passed (4 minutes, 24 seconds)
Details
buildkite/synapse/python-2-dot-7-slash-sqlite-slash-old-deps Passed (5 minutes, 53 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-4 Passed (25 minutes, 33 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (25 minutes, 30 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (4 minutes, 46 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (4 minutes, 55 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (24 minutes, 56 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (25 minutes, 35 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (4 minutes, 51 seconds)
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
codecov/patch 61.65% of diff hit (target 0%)
Details
codecov/project 63.74% (target 0%)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.