Try to relax the 30 sec wait for syncs in the Scenario Player

raiden-network / scenario-player

MIT License

1 stars 16 forks source link

Try to relax the 30 sec wait for syncs in the Scenario Player #577

Closed Dominik1999 closed 4 years ago

Dominik1999 commented 4 years ago

Scenarios take 4 times longer than before

After https://github.com/raiden-network/scenario-player/pull/563 some scenarios, e.g. bf1, take 4 times longer than before, now >4h.

On the other hand, those scenarios seem to be less flaky.

Can we relax the 30s wait?

Investigate whether we can relax the 30s wait to decrease the run time of those scenarios.

there might be tasks that do not need to synchronize 30s after execution
we might be able to reduce the 30s to a lower number (we can use the statistical data we collect since 2 month with every scenario run)

Conclusion of possible reduction and implementation

This is issue is considered to be done, when there is a well-founded conclusion whether we can reduce the time of the scenario runs or not. ~~If we can, then the implementation is part of this issue.~~

karlb commented 4 years ago

This is not only a problem for the SP, but also for other Raiden API users. So it would be great of there was a general approach how to deal with such sychronization problems.

konradkonrad commented 4 years ago

also for other Raiden API users

can you give an example?

palango commented 4 years ago

potential option for synchronization:

add PFS endpoint for network inspection/waiting
add reporting hooks to client code
change assertions to be based on more sophisticated client inspection (API based)

But, the overall question is whether we want to bloat the codebase with synchronization for testing.

palango commented 4 years ago

Check if we can use retry logic instead of a static wait

palango commented 4 years ago

I checked some scenarios and most waits are in front of assertions. This leds to my proposed solution of making all assertions retryable in order to be able to remove the forced sync wait and most of the waits in scenarios as well.

With this approach we'll not be able to get rid of all wait tasks, but I think it's a good compromise between being fast in the happy case and not having to put too much manual work in scenarios.

In https://github.com/raiden-network/scenario-player/pull/453 I moved the retry logic to the base Task class, with retries being disabled there. The logic is only enabled for assert-style tasks like AssertBlockchainEventsTask, AssertPFS*Tasks and Assert*Tasks.

ulope commented 4 years ago

Sounds very reasonable. I think we should try it out.

palango commented 4 years ago

Fixed with https://github.com/raiden-network/scenario-player/pull/453