Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Batch up power level event when creating rooms #14070

Closed
wants to merge 13 commits into from
Closed

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Oct 5, 2022

Follow on work from #13800. Add the initial power level event to the group of events that are batch persisted when the room is created. Reviewable commit-by-commit.

@H-Shay H-Shay requested a review from a team as a code owner October 5, 2022 20:55
@H-Shay H-Shay marked this pull request as draft October 5, 2022 21:06
@H-Shay H-Shay removed the request for review from a team October 5, 2022 21:06
@H-Shay H-Shay self-assigned this Oct 6, 2022
@H-Shay H-Shay marked this pull request as ready for review October 7, 2022 03:02
@H-Shay H-Shay requested a review from a team October 12, 2022 13:55
@erikjohnston erikjohnston assigned erikjohnston and unassigned H-Shay Oct 14, 2022
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me at first glance.

Though a point I wanted to check:
are these events persisted in a chain that all point to each other? I thought selecting the prev_events would happen in create_event, which seems to be happening before the events get persisted...
(Is there a test around that tests that the events created when creating a room are a chain?)

(edit: just thinking that it seems quite important for the power levels event to come before the others)

It also looks like you've collided with the push rules evaluator port to Rust, so a big chunk of your changes need porting :(

@erikjohnston
Copy link
Member

I think it might be helpful to split the batching up of push rules into a separate PR too? A quick glance looks like there are a few different things going on in here.

@H-Shay
Copy link
Contributor Author

H-Shay commented Oct 14, 2022

Though a point I wanted to check: are these events persisted in a chain that all point to each other? I thought selecting the prev_events would happen in create_event, which seems to be happening before the events get persisted... (Is there a test around that tests that the events created when creating a room are a chain?)

The events are indeed persisted in a chain pointing to one another. While there is not an explicit test verifying this, a number of existing tests implicitly rely on this behavior and break in arcane ways when this assumption is violated. I discovered this early on in the process of batching up the events, when I devised a solution which branched the dag (which is not the solution that landed). Currently, the prev_event for each event created in this path is passed in to create_event and that gets passed on down to the builder.

@H-Shay
Copy link
Contributor Author

H-Shay commented Oct 17, 2022

Right I've split up this PR, the first half deals with the auth events and is here: #14214. Once that's approved I will put up a PR dealing with the push rule changes and adding the initial power level event.

@H-Shay H-Shay closed this Oct 17, 2022
@H-Shay
Copy link
Contributor Author

H-Shay commented Oct 18, 2022

The second half is at #14228

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants