Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for received events using /sync in faster room joins tests #441

Merged
merged 2 commits into from
Aug 18, 2022

Conversation

squahtx
Copy link
Contributor

@squahtx squahtx commented Aug 10, 2022

matrix-org/synapse#13477 unblocks lazy-loading
/syncs while a room has partial state, which allows us to wait for
received events using /sync.

"Resync completes even when events arrive before their prev_events" is
now the only test that waits for events using /event, since the
outliers do not appear in the /sync timeline.

@squahtx squahtx requested review from a team as code owners August 10, 2022 16:34
@squahtx
Copy link
Contributor Author

squahtx commented Aug 10, 2022

"Resync completes even when events arrive before their prev_events" is
now the only test that waits for events using /event, since the
outliers do not appear in the /sync timeline.

@richvdh Is this expected behaviour?

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

LGTM - though @richvdh is probably better qualified to review this.

@erikjohnston
Copy link
Member

@squahtx is this just waiting for @richvdh's input now?

@richvdh richvdh requested review from richvdh and removed request for a team August 17, 2022 09:08
@richvdh
Copy link
Member

richvdh commented Aug 17, 2022

"Resync completes even when events arrive before their prev_events" is
now the only test that waits for events using /event, since the
outliers do not appear in the /sync timeline.

@richvdh Is this expected behaviour?

It's expected that outliers don't come down /sync, if that's what you're asking?

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm!

getSyncToken := func(t *testing.T, alice *client.CSAPI) string {
_, syncToken := alice.MustSync(t,
client.SyncReq{
Filter: buildLazyLoadingSyncFilter(nil),
Copy link
Member

Choose a reason for hiding this comment

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

I think you probably don't need a Filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't that make the function block if we decide to use it midway through a partial join?

Copy link
Member

Choose a reason for hiding this comment

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

yes, but I didn't think this was used in the middle of a partial-state join?

(indeed, I thought initialsyncs in the middle of partial-state joins blocked irrespective of the lazy-loading flag. I'm probably wrong though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't currently used in the middle of a partial-state join, but I would like to keep the option open.

lazy-loading initial syncs don't (or no longer?) block during a partial-state join, otherwise one of the new lazy-loading tests in #442 would be getting stuck.

Copy link
Member

Choose a reason for hiding this comment

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

all makes sense, thanks!

@squahtx squahtx merged commit 708348a into main Aug 18, 2022
@squahtx squahtx deleted the squah/faster_room_joins_unblock_lazy_loading_sync_1 branch August 18, 2022 14:29
squahtx pushed a commit that referenced this pull request Aug 18, 2022
squahtx added a commit that referenced this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants