protocols/kad/query/peers/closest: Make at_capacity use max #1548
Conversation
When not making progress for `parallelism` time a `ClosestPeersIter` becomes `State::Stalled`. When stalled an iterator is allowed to make more parallel requests up to `num_results`. If `num_results` is smaller than `parallelism` make sure to still allow up to `parallelism` requests in-flight.
| assert!(!iter.at_capacity()); | ||
| } | ||
|
|
||
| /// When not making progress for `parallelism` time a [`ClosestPeersIter`] becomes |
There was a problem hiding this comment.
This test is likely a bit over the top. I left it here to showcase the issue in a more in-depth scenario. I would suggest removing it before merging. What do you think?
There was a problem hiding this comment.
Yes, I don't think it adds much to the other test, especially if the other is turned into a property test. Thanks for writing it though.
There was a problem hiding this comment.
An interesting edge-case, though unlikely to be hit in practice as num_results for the iterator is taken from the configured replication factor (K_VALUE by default) and I doubt someone configures it to be < ALPHA = 3. Nevertheless, this is a bug (at least after #1536) 👍.
| @@ -691,4 +693,78 @@ mod tests { | |||
|
|
|||
| QuickCheck::new().tests(10).quickcheck(prop as fn(_) -> _) | |||
| } | |||
|
|
|||
| #[test] | |||
| fn stalled_iter_at_capacity_use_max_of_parallelism_and_num_results() { | |||
There was a problem hiding this comment.
Wouldn't this make for a good property test? The property being the following: "For any ClosestPeersIter, if it is in State::Stalled then it must be at capacity only if num_waiting = max{num_results, parallelism}" ?
| assert!(!iter.at_capacity()); | ||
| } | ||
|
|
||
| /// When not making progress for `parallelism` time a [`ClosestPeersIter`] becomes |
There was a problem hiding this comment.
Yes, I don't think it adds much to the other test, especially if the other is turned into a property test. Thanks for writing it though.
Remove in-depth test and make smaller test property based.
|
Property based test is a good idea. Would you mind taking another look @romanb? |
Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>
When not making progress for
parallelismtime aClosestPeersIterbecomes
State::Stalled. When stalled an iterator is allowed to makemore parallel requests up to
num_results. Ifnum_resultsis smallerthan
parallelismmake sure to still allow up toparallelismrequestsin-flight.