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.
Faster joins: persist to database #12012
Faster joins: persist to database #12012
Changes from 4 commits
e1d95e8
36f8a6a
9f34681
57ff630
ad0078f
f499f40
446fae6
e3c84ff
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 unclear on why we need to do this again when de-outliering. Does de-outliering not imply that the event has already been through this code once?
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.
seems to be another great question. I'll try and remember what's going on here.
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.
Right, I've remembered what this is about.
Outliers have no state at all, so it's meaningless to say that they have "partial state". So, the first time they go through this code, we would not expect to set the partial-state flag.
When we de-outlier the event, it's possible that we might de-outlier it with only partial state, and in that case we need to add a row to
partial_state_events
when we de-outlier it, which is why I've put this code here.I've added a robustness check to make sure it's true that outliers are never flagged with partial-state in 446fae6.
I've also got it on my todo list to make sure we end up with tests which include de-outliering events with partial 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.
Thanks for the explanation! So the outliers we persist will never be flagged with partial state because we always use
EventContext.for_outlier()
as the context*?* with the exception of the outlier in
RoomMemberMasterHandler._generate_local_out_of_band_leave
for reasons that are beyond me right 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.
Yeah, that looks a bit bogus. It means we end up recording state at the leave event, but also flagging it as an outlier, which seems wrong. Either it shouldn't be an outlier, or we shouldn't record the state at it. I'm going to park this for now though.
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.
for the record: I've ended up with a fix to this, at #12154, as it broke in the face of my fixes to #12074.