This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add pagination support to publicRooms #1121
Merged
Merged
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
LGTM <- not sure why I typed that yet. I should probably actually read the PR... :( |
FROM public_room_list_stream | ||
WHERE stream_id <= ? | ||
GROUP BY room_id | ||
) grouped USING (room_id, stream_id) |
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.
Will this be quick on postgres, or will you need to use window functions there?
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.
Not that it really matters since the table will probably be small to start with.
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.
I've been testing on postgres, and it appears to use the right indices.
@matrixbot retest this please |
LGTM |
This was referenced Sep 15, 2016
Closed
3 tasks
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.
Changes:
start
andend
keys have been removed from the response to/publicRooms
, and replaced bynext_batch
andprev_batch
(whose existence depends on if there is a next or prev batch)./publicRooms
acceptlimit
andsince
parameters.{next,prev}_batch
tokens tosince=
. There is nodirection
parameter.since
token.new_rooms
is returned in the response when a since token is specified and is a boolean indicating if there are new rooms. Unpublished rooms do get removed, however.limit
param is advisory, in particular synapse will return fewer rooms if they have been unpublished.The response now looks something like:
Question: If we don't specify a limit, should we return the full list (current behaviour) or should we cap it?