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.
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
Draft: Test that
/messages
works on remote homeserver and can be backfilled properly after many batches (MSC2716) #214Draft: Test that
/messages
works on remote homeserver and can be backfilled properly after many batches (MSC2716) #214Changes from 17 commits
8099f47
094c5f7
e30bcd4
1e333d6
2022b31
d325349
2fe5180
0604564
83adbe2
4aba836
ffbca43
37109fa
cc7236b
4c8284a
9b90429
677836b
85eb7bd
3532821
1667e15
0589546
606197a
d679384
230c46e
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.
This timeout is currently very large to accommodate the long ~20s
/messages
durations. We also have to make that request 11 times during the test which adds up very fast.Synapse really has to chug for those requests 👹 and ideally wouldn't have to modify this at all.
I would need to look into optimizing Synapse to make this fast which we should probably do anyway as this is painfully slow.
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.
This is currently set to a whopping 1000s by default
complement/internal/docker/deployment.go
Line 57 in 136fd60
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.
@kegsay Sorry this wasn't clear as undrafting indicates "marked this pull request as ready for review" but I didn't assign you this one yet specifically because of this problem.
The test itself is good to go (timeout can be switched to normal and
numBatches
could be switched back to11
) and shipped but want to make it actually acceptable time-wise to run against Synapse before merging.I used
numBatches=2
during testing because it's much faster to see results while developing.Thanks for the review pass though and I'll fix up these other spots ⏩
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.
Status is still the same as the last update in this thread. It's too slow on Synapse for me to be comfortable merging it 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.
In terms of optimizing Synapse to make this test viable to run time-wise, I'm a bit blocked on a race condition in some recent code, matrix-org/synapse#12394 (comment) -> matrix-org/synapse#12646
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.
Now matrix-org/synapse#12988 I think
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.
Made progress on optimizing Synapse:
/messages
call: Fixhave_seen_event
cache not being invalidated synapse#13863prev_event
thrashing (scratch) synapse#13864There 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.
👆 Calling out a change since the last review pass. I added a
denyListEventIDs
so we can ensure that the historical state doesn't appear between batches of historical messages while we're paginating.