This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
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.
Add forward extremities endpoint to rooms admin API #9062
Add forward extremities endpoint to rooms admin API #9062
Changes from 7 commits
b849e46
c91045f
85c0999
90ad4d4
2eb421b
e2c16ed
b52fb70
0b77329
da16d06
49c619a
c177faf
930ba00
8965b6c
fe18882
fdf8346
e20f18a
cee4010
4936fc5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 thingThere 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 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 comment
The 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
SynapseError
? I didn't do it since I saw some other places use the SynapseError in db code.