-
Notifications
You must be signed in to change notification settings - Fork 404
Implement MSC3871: Gappy timelines - indicate gaps in /messages
#18873
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
base: develop
Are you sure you want to change the base?
Conversation
See [MSC: Gappy timelines](matrix-org/matrix-spec-proposals#3871)
To try out the flow:
- **Default to fast responses with gaps**: As a default, we can always
respond quickly and indicate gaps ([MSC3871]
(matrix-org/matrix-spec-proposals#3871)) for
clients to paginate at their leisure.
- **Fast back-pagination**: Clients back-paginate with
`/messages?dir=b&backfill=false`, and Synapse skips backfilling
entirely, returning only local history with gaps as necessary.
- **Explicit gap filling**: To fill in gaps, clients use
`/messages?dir=b&backfill=true` which works just like today to do a best
effort backfill.
This allows the client to back-paginate the history we already have without
delay. And can fill in the gaps as they see fit.
This is basically a simplified version of [MSC4282]
(matrix-org/matrix-spec-proposals#4282).
| def get_stream_pos_for_instance(self, instance_name: str) -> int: | ||
| """Get the stream position that the given writer was at at this token. | ||
| def is_before_or_eq(self, other_token: Self) -> bool: | ||
| is_before_or_eq_stream_ordering = super().is_before_or_eq(other_token) | ||
| if not is_before_or_eq_stream_ordering: | ||
| return False | ||
|
|
||
| This only makes sense for "live" tokens that may have a vector clock | ||
| component, and so asserts that this is a "live" token. | ||
| """ | ||
| assert self.topological is None |
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.
Removed get_stream_pos_for_instance because there is no need to have this specialized version that only allows stream-based tokens. We will fallback to the super() version which is the same but without the topological restriction.
While only "live" tokens may have a vector clock component, historical topological tokens still include a stream position (t426-2633508) and it makes sense for this to still work.
synapse/synapse/types/__init__.py
Lines 697 to 703 in 68068de
| Historic tokens start with a "t" followed by the `depth` | |
| (`topological_ordering` in the event graph) of the event that comes before | |
| the position of the token, followed by "-", followed by the | |
| `stream_ordering` of the event that comes before the position of the token. | |
| An example token is: | |
| t426-2633508 |
…mate comparisons
| It's best to pad the `current_depth` by the number of messages you plan to | ||
| backfill from these points. |
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.
This is a good useful change that we could ship outside of this PR.
Noting in case this PR goes stale
| # from synapse.storage.databases.main.stream import ( | ||
| # generate_next_token, | ||
| # ) |
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.
| # from synapse.storage.databases.main.stream import ( | |
| # generate_next_token, | |
| # ) |
| ignore_gap_after_latest: Whether the gap after the latest events (forward | ||
| extremeties) in the room should be considered as an actual gap. |
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.
Per matrix-org/matrix-spec-proposals#3871 (comment), we should revert the ignore_gap_after_latest change.
The default of "omit the gap after the latest messages in the room" is the correct choice
Implements MSC3871: Gappy timelines
gapsfield to the/messagesendpointComplement tests: matrix-org/complement#801
Motivating use case
Testing strategy
synapsegit checkout madlittlemods/msc3871-gappy-timeline(this PR)poetry installcomplementnext tosynapseas a siblinggit checkout madlittlemods/msc3871-gappy-timeline(Tests for MSC3871: Gappy timelines matrix-org/complement#801)In order to manually test with a real Matrix client:
complement, uncomment thesleepcall inTestRoomMessagesGapsCOMPLEMENT_DIR=../complement ./scripts-dev/complement.sh -run TestRoomMessagesGapshs3is running on:docker ps -f name=complement_(ex.0.0.0.0:33413->8008/tcp)http://localhost:33413/messages?dir=b&backfill=falseand notice thegapsin the room/messages?dir=b&backfill=true&from=<prev_pagination_token>Reference: Complement docs on how to hook up a Matrix client
Dev notes
Document how to hook up Element to the resultant homeservers from Complement: matrix-org/complement#164
matrix-org/complement->ONBOARDING.md-> How do I hook up a Matrix client like Element to the homeservers spun up by Complement after a test runs?Todo
test_gaps_going_forwards/messages?backfill=true/falsechanges (simplified version of MSC4282)Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.