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 forward extremities endpoint to rooms admin API #9062
Merged
Merged
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
b849e46
Add forward extremities endpoint to rooms admin API
jaywink c91045f
Move unknown room ID error into resolve_room_id
jaywink 85c0999
Add Rooms admin forward extremities DELETE endpoint
jaywink 90ad4d4
Implement clearing cache after deleting forward extremities
jaywink 2eb421b
Merge branch 'develop' into jaywink/admin-forward-extremities
jaywink e2c16ed
Add changelog and admin API docs
jaywink b52fb70
Don't try to use f-strings
jaywink 0b77329
Clarify rooms.md
jaywink da16d06
Address pr feedback
jaywink 49c619a
Simplify delete_forward_extremities_for_room_txn SQL
jaywink c177faf
Remove trailing whitespace to appease the linter
jaywink 930ba00
Add depth and received_ts to forward_extremities admin API response
jaywink 8965b6c
Merge branch 'develop' into jaywink/admin-forward-extremities
jaywink fe18882
Merge remote-tracking branch 'origin/develop' into jaywink/admin-forw…
jaywink fdf8346
Merge remote-tracking branch 'origin/develop' into jaywink/admin-forw…
clokep e20f18a
Make natural join inner join
jaywink cee4010
Merge branch 'develop' into jaywink/admin-forward-extremities
jaywink 4936fc5
Fix get forward extremities query
jaywink File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add admin API for getting and deleting forward extremities for a room. |
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
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
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
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
101 changes: 101 additions & 0 deletions
101
synapse/storage/databases/main/events_forward_extremities.py
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
# -*- coding: utf-8 -*- | ||
# Copyright 2021 The Matrix.org Foundation C.I.C. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import logging | ||
jaywink marked this conversation as resolved.
Show resolved
Hide resolved
|
||
from typing import Dict, List | ||
|
||
from synapse.api.errors import SynapseError | ||
from synapse.storage._base import SQLBaseStore | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class EventForwardExtremitiesStore(SQLBaseStore): | ||
async def delete_forward_extremities_for_room(self, room_id: str) -> int: | ||
"""Delete any extra forward extremities for a room. | ||
|
||
Invalidates the "get_latest_event_ids_in_room" cache if any forward | ||
extremities were deleted. | ||
|
||
Returns count deleted. | ||
""" | ||
|
||
def delete_forward_extremities_for_room_txn(txn): | ||
# First we need to get the event_id to not delete | ||
sql = """ | ||
SELECT event_id FROM event_forward_extremities | ||
INNER JOIN events USING (room_id, event_id) | ||
WHERE room_id = ? | ||
ORDER BY stream_ordering DESC | ||
LIMIT 1 | ||
""" | ||
txn.execute(sql, (room_id,)) | ||
rows = txn.fetchall() | ||
try: | ||
event_id = rows[0][0] | ||
logger.debug( | ||
"Found event_id %s as the forward extremity to keep for room %s", | ||
event_id, | ||
room_id, | ||
) | ||
except KeyError: | ||
msg = "No forward extremity event found for room %s" % room_id | ||
logger.warning(msg) | ||
raise SynapseError(400, msg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we usually try to not raise synapse errors from the database layer, but I'm not finding a much better solution at the moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I felt annoyed at this too, but didn't feel there was much to do, without separating the queries into separate transactions at least. I guess one thing could be raising some non-http error and then catching that in the servlet to raise a |
||
|
||
# Now delete the extra forward extremities | ||
sql = """ | ||
DELETE FROM event_forward_extremities | ||
WHERE event_id != ? AND room_id = ? | ||
""" | ||
|
||
txn.execute(sql, (event_id, room_id)) | ||
logger.info( | ||
"Deleted %s extra forward extremities for room %s", | ||
txn.rowcount, | ||
room_id, | ||
) | ||
|
||
if txn.rowcount > 0: | ||
# Invalidate the cache | ||
self._invalidate_cache_and_stream( | ||
txn, self.get_latest_event_ids_in_room, (room_id,), | ||
) | ||
|
||
return txn.rowcount | ||
|
||
return await self.db_pool.runInteraction( | ||
"delete_forward_extremities_for_room", | ||
delete_forward_extremities_for_room_txn, | ||
) | ||
|
||
async def get_forward_extremities_for_room(self, room_id: str) -> List[Dict]: | ||
"""Get list of forward extremities for a room.""" | ||
|
||
def get_forward_extremities_for_room_txn(txn): | ||
sql = """ | ||
SELECT event_id, state_group, depth, received_ts | ||
FROM event_forward_extremities | ||
INNER JOIN event_to_state_groups USING (event_id) | ||
INNER JOIN events INNER JOIN USING (event_id) | ||
jaywink marked this conversation as resolved.
Show resolved
Hide resolved
|
||
WHERE room_id = ? | ||
""" | ||
|
||
txn.execute(sql, (room_id,)) | ||
return self.db_pool.cursor_to_dict(txn) | ||
|
||
return await self.db_pool.runInteraction( | ||
"get_forward_extremities_for_room", get_forward_extremities_for_room_txn, | ||
) |
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.
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.
What sort of situations would you use this in? Is the state group useful?
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.
No, probably not particularly, except maybe for developers. I did think of returning just a count, but it felt that since the information "is there", it felt unnecessary to not return results as well.
My understanding of the needs for this API endpoint from an operational point of view is mainly to see the count, though I would maybe check with the EMS ops team first to confirm.
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 suspect that's just internal state then and not worth returning. @erikjohnston do you see any value in returning those?
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 sometimes used the state group as a proxy for how big the state difference was between extremities were, but it's a bad heuristic. I think a more useful one would probably be
depth
andreceived_ts
, as that'll give you a better way of looking for stuck extremitiesThere 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.
Do we consider those to be useful in this admin API? I guess the options are:
state_group
state_group
, adddepth
andreceived_ts
I don't have enough experience on the extremities to vote really.
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 think adding
depth
andreceived_ts
is probably the right thing