Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ffuugoo
left a comment
There was a problem hiding this comment.
Overall, LGTM. Maybe add explicit yield_now().await in between iterations.
there are about 100 usage in sync tests, it would be a huge refactoring |
This comment was marked as resolved.
This comment was marked as resolved.
…rhead created by spikes
32257ea to
d4da977
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Roman Titov <ffuugoo@users.noreply.github.com>
| // Release some kept clocks | ||
| kept_clocks.retain(|(keep_for, _)| *keep_for > 1); | ||
| kept_clocks.retain_mut(|(keep_for, _)| { | ||
| if *keep_for == 0 { | ||
| return false; | ||
| } | ||
| *keep_for -= 1; | ||
| true | ||
| }); |
There was a problem hiding this comment.
This artificial release mechanism in the test was broken, as it did not decrease the counter.
I've also swapped the number of rounds we keep locks for in the above two loops. Now the smaller loop keeps clocks for longer, while the bigger loop keeps locks for shorter. This way we stay below the specified maximum of 256 clocks and prevent the test panicking.
I believe it's a reasonable way to make the test happy.
* [manual] limit number of update clocks to 64 to prevent permanent overhead created by spikes * explicit yield + fix tests * Add explicit comment on why we yield async thread * In resolve_wal_delta_randomized test fix clock cleanup and satisfy test * Use tokio timeout Co-authored-by: Roman Titov <ffuugoo@users.noreply.github.com> --------- Co-authored-by: Tim Visée <tim+github@visee.me> Co-authored-by: timvisee <tim@visee.me> Co-authored-by: Roman Titov <ffuugoo@users.noreply.github.com>
| match self.clock_set.lock().await.get_clock() { | ||
| Some(clock) => return clock, | ||
| // Prevent blocking async runtime with spinlock | ||
| None => yield_now().await, |
There was a problem hiding this comment.
We may have a condvar that we can await on.
There was a problem hiding this comment.
There is many clocks. We should not wait with a condvar on all of them.
Having a separate atomic or notification channel is possible, but it makes the structure more complicated.
We don't expect to hit this branch under normal circumstances. Only when an excessive amount of parallelism is used, which is a problem on its own.
Clocks are used to track partial ordering of updates across whole cluster. Number of clocks limit the total update parallelism of individual peer, but this limitation seems to acceptable in order to prevent clogging state with too many clocks