-
Notifications
You must be signed in to change notification settings - Fork 86
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
Review of pruning #3495
Review of pruning #3495
Conversation
580207f
to
9afc8e0
Compare
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.
LGTM Very nice! :D
bors merge |
3495: Review of pruning r=coot a=coot Fixes #3487. - pruning: present only inbound connections to the pruning policy - pruning: fix typos - pruning: include current connection in the choice map - pruning: improved logging - pruning: set connection state to TerminatedState - pruning: factor out pruning - pruning: do not prune in Duplex → InboundState transition - pruning: improved a comment Co-authored-by: Marcin Szamotulski <profunctor@pm.me>
bors cancel |
Canceled. |
@@ -2508,6 +2513,19 @@ prop_never_above_hardlimit serverAcc | |||
) | |||
. property | |||
$ incomingConns cmc <= fromIntegral hardlimit | |||
(TrPruneConnections prunnedSet numberToPrune choiceSet) -> |
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.
I'd prefer a separate test for this. It will make it easier to change the counters test to use the Signal API and the pruning test doesn't require it.
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.
I thought about making a separate test but I decided against it because we have to be careful for the test suite to not take too much time to run. This test runs in about 30sec, if we duplicate it will double the time. Still I like to keep properties that test different thing separate as much as possible, whether this will be possible is not yet clear. Maybe, we'll need to optimise io-sim
at some point. As for signal api, it might not be worth it in this test, the condition is precise and clear as it stands.
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.
Fair enough!
0fa66ed
to
d936e3c
Compare
Also isn't it worth it to update the docs in this PR? |
Neil is reviewing the documentation right now, so it's better not to interfere with his work. |
This guarantees that whatever choice the policy will make, it will make progress towards smaller number of inbound connections.
When pruning connections we have to add current connection, as it is filtered out by the guard.
When pruning we can set the state to TerminatedState and then cancel the connection handler thread, which will recognise this connection to be pruned. Note that this avoids our application level WAIT_TIME interval.
When we unregister outbound side of a Duplex connection, it does not changes the number of inbound connections, so there's not need to prune anything.
d936e3c
to
7ba155a
Compare
bors merge |
Build succeeded: |
Fixes #3487.