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 3 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.
Lovely comment but then you seem to use 2. Why?
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.
See:
Running this test against Synapse is extremely slow atm, #214 (comment)
TODO: Reminder to revert this to 11 before merge
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.
So this is new -> old?
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.
The order doesn't matter because it's just a list of expected ID's,
expectedEventIDs
We also just use it in check off function (
paginateUntilMessageCheckOff
) which doesn't care about the order.There is a similar variable in another test where order does matter but we call it
expectedEventIDOrder
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.
Why not use
JSONArrayEach
?complement/internal/match/json.go
Line 164 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.
IIRC, I think it was because we can't break early with
JSONArrayEach
(can only break early if you throw an error which fails the test).JSONArrayEach
could be refactored to have the item function returnkeepGoing, err
but rather do that in another PR since it touches so many other tests.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.
Is the early return really a deal breaker? I don't think it'll affect runtime performance that much?
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.
Updated to re-use
JSONArrayEach
. It's not so bad ⏩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.
So this function doesn't actually check the events are in the correct order, just that they exist?
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.
Correct, just a checkoff function