This repository has been archived by the owner on Nov 14, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 676
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
doesn't yet compile or work. needs to actually add the peeking block into the sync response. checking in now before it gets any bigger, and to gather any initial feedback on the vague shape of it.
kegsay
reviewed
Sep 1, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broadly speaking this implements MSC2753 well. There's some discussion to be had around the shape of peeking over federation, which will depend on what the federation API shape looks like.
To use: set `DENDRITE_TRACE_SQL=1` then grep for `unsafe`
assuming CI passes, i think this is ready to merge. |
anoadragon453
pushed a commit
to matrix-org/synapse
that referenced
this pull request
Sep 9, 2020
Dendrite's implementing MSC2753 over at matrix-org/dendrite#1370 to prove the implementation for MSC purposes, and so sytest has sprouted tests for it over at matrix-org/sytest#944. But we don't want them to run on synapse until synapse implements it.
…ly altered sp when nothing is deleted
kegsay
reviewed
Sep 9, 2020
sytest is still failing for me locally on postgres with the peeked blocks having no timeline entries, so i am very confused on how this is passing CI. |
kegsay
reviewed
Sep 10, 2020
kegsay
approved these changes
Sep 10, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM!
neilalexander
approved these changes
Sep 10, 2020
clokep
pushed a commit
to matrix-org/synapse
that referenced
this pull request
Sep 17, 2020
Dendrite's implementing MSC2753 over at matrix-org/dendrite#1370 to prove the implementation for MSC purposes, and so sytest has sprouted tests for it over at matrix-org/sytest#944. But we don't want them to run on synapse until synapse implements it.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft PR to gather feedback on the vague shape (and to help inform review of MSC2753), and to get something committed.
Full scope (although i'd like to merge this before it bitrots and handle the other checkboxes in other PRs):
peek
block into the sync response/unpeek
leave
blockSee also matrix-org/matrix-spec-proposals#2753
This consciously doesn't implement peeking over federation, which should be a separate PR, but is the main reason for doing this.