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

Peeking via MSC2753 #1370

Merged
merged 66 commits into from
Sep 10, 2020
Merged

Peeking via MSC2753 #1370

merged 66 commits into from
Sep 10, 2020

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Aug 30, 2020

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):

  • get feedback on whether the design is remotely correct
  • make it build
  • make it work
  • actually add peek block into the sync response
  • respect history visibility
  • add postgres
  • add /unpeek
  • show unpeeked rooms briefly in the leave block
  • support EDUs in peeked rooms
  • figure out whether to stop trying to wake up devices which are no longer syncing
  • stop peeking if history visibility changes
  • clean up peeks on since-less /sync
  • sort out unstable prefixes
  • tracing
  • complement / sytests - sytest in progress at test peeking via MSC2753 on dendrite sytest#944

See 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.

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.
@ara4n ara4n marked this pull request as draft August 30, 2020 14:52
Copy link
Member

@kegsay kegsay left a 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.

clientapi/routing/routing.go Outdated Show resolved Hide resolved
docs/peeking.md Show resolved Hide resolved
docs/peeking.md Show resolved Hide resolved
roomserver/api/output.go Show resolved Hide resolved
syncapi/storage/shared/syncserver.go Outdated Show resolved Hide resolved
syncapi/sync/notifier.go Show resolved Hide resolved
@ara4n
Copy link
Member Author

ara4n commented Sep 9, 2020

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.
@ara4n
Copy link
Member Author

ara4n commented Sep 10, 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.

@ara4n ara4n closed this Sep 10, 2020
@ara4n ara4n reopened this Sep 10, 2020
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM!

@ara4n ara4n merged commit 39507ba into master Sep 10, 2020
@ara4n ara4n deleted the matthew/peeking branch September 10, 2020 13:39
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 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