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

Spec lazy-loading room members #2035

Merged
merged 19 commits into from Jun 11, 2019

Conversation

Projects
None yet
6 participants
@turt2live
Copy link
Member

commented May 28, 2019

Reviewers: Although this has team review requested, I would appreciate particular review from @ara4n as the proposal author and @KitsuneRal as having gone through the gauntlet. Feedback in addition is more than appreciated.

Disclaimer: Although the implementation is lacking in areas (see below), it is believed by the spec core team that lazy-loading has enough benefits to be added to the spec, as per the MSC guidelines.


Implementation references:

Notable lacks of implementation:

Spec references:

Alterations from the proposals:

  • The MSCs are unclear of what scope "redundant" is defined under. Written as "across any of the supported endpoints".
  • Clarification that servers and clients should be careful when dealing with historical data. Unsure how Synapse/Riot handle this, if at all.
  • Unclear if summary should be required. The implementation only does so when lazy-loading is active, however this seems like a bug. Written as required so that clients can use m.heroes to name the room.

(note: due to these proposals being older Google Doc MSCs, they have had the alterations described here represented in the applicable threaded comments instead of the body of the proposal. This is to hopefully more clearly represent the changes.)

turt2live added some commits May 27, 2019

Move lazy loading filter options to event filter
The options also work on /messages
Clarify wording further for how to handle redundant members
Note: This makes assumptions on what the TODO comment in Synapse means: https://github.com/matrix-org/synapse/blob/e26e6b3230f0b55376f0f3bf823dd789ac7064d0/synapse/handlers/pagination.py#L262

Due to lack of implementation, it is assumed that using the same filter across multiple calls to /sync OR /messages will result in the redundant members being excluded in the next request. For example, calling /sync, then /messages which returns some members, then /sync again will exclude the members due to them being in /messages.
Show resolved Hide resolved api/client-server/sync.yaml Outdated
@richvdh
Copy link
Member

left a comment

generally looks good. I'm a bit hazy about some of this stuff though :/

Show resolved Hide resolved specification/client_server_api.rst
Show resolved Hide resolved api/client-server/event_context.yaml Outdated
Show resolved Hide resolved api/client-server/message_pagination.yaml Outdated
Show resolved Hide resolved api/client-server/rooms.yaml
Show resolved Hide resolved api/client-server/sync.yaml Outdated
Information about the room which clients may need to
correctly render it to users.
properties:
"m.heroes":

This comment has been minimized.

Copy link
@richvdh

richvdh Jun 6, 2019

Member

are these properties not required ?

This comment has been minimized.

Copy link
@turt2live

turt2live Jun 7, 2019

Author Member

no, because if they are omitted then the client should interpret it as "no change".

Show resolved Hide resolved specification/client_server_api.rst Outdated
Show resolved Hide resolved api/client-server/definitions/room_event_filter.yaml Outdated
Show resolved Hide resolved specification/client_server_api.rst Outdated
Show resolved Hide resolved specification/modules/instant_messaging.rst Outdated

turt2live and others added some commits Jun 7, 2019

Apply suggestions from code review
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

@turt2live turt2live requested a review from richvdh Jun 7, 2019

Show resolved Hide resolved api/client-server/message_pagination.yaml Outdated
Show resolved Hide resolved api/client-server/message_pagination.yaml Outdated
Show resolved Hide resolved api/client-server/message_pagination.yaml Outdated
Show resolved Hide resolved specification/client_server_api.rst Outdated
Show resolved Hide resolved api/client-server/sync.yaml Outdated
Show resolved Hide resolved api/client-server/sync.yaml Outdated
Show resolved Hide resolved api/client-server/sync.yaml Outdated
Show resolved Hide resolved specification/client_server_api.rst Outdated
Show resolved Hide resolved specification/client_server_api.rst Outdated
Show resolved Hide resolved specification/client_server_api.rst Outdated
Apply suggestions from code review
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Show resolved Hide resolved api/client-server/sync.yaml Outdated

ara4n and others added some commits Jun 10, 2019

Merge pull request #2106 from matrix-org/matthew/1.0/msc688-msc1227-l…
…azy-loading

incorporate LL review from matthew

@turt2live turt2live requested a review from matrix-org/spec-core-team Jun 10, 2019

@turt2live

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

Shoving this back into the queue to get a checkmark (hopefully) from someone on the team. There's a few people who have reviewed this, so picking on someone isn't super obvious.

@ara4n

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

i'm happy with it from my pov fwiw, but i guess my rewrite needs some review if someone can face it.

@turt2live

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

arguably this is fine to merge as I reviewed your edits, but I think a third opinion would be good given the amount of review on here.

@ara4n

ara4n approved these changes Jun 11, 2019

@ara4n

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

let's just merge it.

@turt2live turt2live merged commit 19f017f into master Jun 11, 2019

8 checks passed

buildkite/matrix-doc Build #296 passed (57 seconds)
Details
ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

@turt2live turt2live deleted the travis/1.0/msc688-msc1227-lazy-loading branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.