Run multiple event persisters when using Redis #972
Conversation
|
@erikjohnston Looks like this has some failing tests! |
| })->then( sub { | ||
| # Check public rooms list for local user | ||
| my ( $body ) = @_; | ||
| retry_until_success { |
There was a problem hiding this comment.
why are these changes necessary? And if they are really necessary, can we limit the number of times we retry, rather than going on forever?
There was a problem hiding this comment.
The test was flakey. This may no longer be necessary, but in general I don't think the public room lists get updated immediately (as they can be populated via background processes).
The retry_until_success will get killed by the test timeout, which is how we use retry_until_success/repeat_until_true elsewhere?
There was a problem hiding this comment.
could you add a comment to say that we're retrying because the directory is updated asynchonously, or sth.
My main gripe with retry_until_success is that it turns test failures into test timeouts, which we tend to ignore as "oh we had a slow VM for random reasons", so a limit on the number of retries is preferable if possible.
The other problem with retry_until_success is that it swallows the failure, so you can't see what's going wrong. https://github.com/matrix-org/sytest/blob/develop/tests/30rooms/04messages.pl#L387-L408 includes a bunch of code to log what's going on, but on closer inspection maybe we should put all that boilerplate into retry_until_success (which could maybe also benefit from a "try N times" param which defaults to something other than "infinity").
There was a problem hiding this comment.
Have added a comment.
Yeah, I think that sounds like a general issue with retry_until_success. We use this pattern a lot so it feels like something to investigate and improve separately.
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
For the second time this week (#972 and #973), I've found myself wondering why a test was failing and discovered that it is due to it relying on new functionality in Future 0.45. I guess we may as well bump the dep as try to keep our tests backwards-compatible, particularly given the CI uses the latest version.
For the second time this week (#972 and #973), I've found myself wondering why a test was failing and discovered that it is due to it relying on new functionality in Future 0.45. I guess we may as well bump the dep as try to keep our tests backwards-compatible, particularly given the CI uses the latest version.
This makes SyTest test Synapse's sharded event persister set up.
Requires matrix-org/synapse#8499