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.
Resync state after partial-state join #12394
Resync state after partial-state join #12394
Changes from 1 commit
8ff5f48
235f0bf
4dafba8
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.
I'm guessing we need to persist more info to the database and then ensure that starts up again on restart?
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'm not even sure we ned any extra info in the database - all we need is a list of room ids, and we have that.
So yes, it's just a matter of starting it up again on restart.
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.
Fair! Was mostly just curious, not asking for it to be fixed yet! 👍
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.
Maybe put this on the background process worker?
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, maybe. On the other hand it does a bunch of work which is quite similar to the event creator, and it would be nice to shard it across multiple workers (as we do with event creators), so it's not entirely obvious. Either way, I'm punting the decision for now.
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.
Maybe this is fine, but will this work OK if a second user joins from your server while we only have partial room state?
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.
that's really a special-case of the more general problem of sending events while we have partial state. Currently, it's completely broken, but in a couple of PRs time, we'll have a thing which will wait for the resync to complete.
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.
That makes sense, thanks! 👍 Glad to know this is something in the works already.
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'm a bit confused at where this might come from -- if we've resolved the entire room state how could there be states with partial state arriving?
These are from other servers in the room sending us events, not from the process of resolving partial state, correct?
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.
that's right. Currently, when we get a new event over federation, we calculate the state at that event based on the state at the new event's
prev_event(s)
. Obviously, if any of thoseprev_events
have partial state, the state at the new event is also going to be partial. So, there is potential for a race:X
is the only one left. We start the process of updating the state atX
.Y
arrives which refers to eventX
in its prev_events. At this point,X
still has partial state, hence so doesY
. We hand offY
to be persisted by the event persistence thread.X
, and go round the loop again. We find there are no remaining partial state events. Hurrah!Y
gets persisted.Obviously, there's another failure mode here, where
Y
's persistence happens after we mark the room as having no partial-state events. It seems like I'm going to need to revisit this. For now, I'm just going to add yet another TODO.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.
It seems unfortunate that these will be called separately per event, but I don't see a good way to make this method take a list of events.
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 we think about optimising it later. Hopefully there won't be a huge number of events anyway.