Skip to content
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

Merged
merged 9 commits into from
Nov 22, 2021
Merged

Review of pruning #3495

merged 9 commits into from
Nov 22, 2021

Conversation

coot
Copy link
Contributor

@coot coot commented Nov 19, 2021

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

@coot coot added networking connection-manager Issues / PRs related to connection-manager labels Nov 19, 2021
@coot coot force-pushed the coot/never-above-hardlimit-failure branch from 580207f to 9afc8e0 Compare November 19, 2021 10:47
@coot coot requested a review from bolt12 November 19, 2021 13:44
@coot coot marked this pull request as ready for review November 19, 2021 13:44
@coot coot requested a review from karknu as a code owner November 19, 2021 13:44
Copy link
Contributor

@bolt12 bolt12 left a 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

@coot
Copy link
Contributor Author

coot commented Nov 19, 2021

bors merge

iohk-bors bot added a commit that referenced this pull request Nov 19, 2021
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>
@coot
Copy link
Contributor Author

coot commented Nov 19, 2021

bors cancel

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 19, 2021

Canceled.

@@ -2508,6 +2513,19 @@ prop_never_above_hardlimit serverAcc
)
. property
$ incomingConns cmc <= fromIntegral hardlimit
(TrPruneConnections prunnedSet numberToPrune choiceSet) ->
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

@coot coot force-pushed the coot/never-above-hardlimit-failure branch from 0fa66ed to d936e3c Compare November 19, 2021 16:34
@bolt12
Copy link
Contributor

bolt12 commented Nov 19, 2021

Also isn't it worth it to update the docs in this PR?

@coot
Copy link
Contributor Author

coot commented Nov 22, 2021

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.

coot added 9 commits November 22, 2021 11:27
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.
@coot coot force-pushed the coot/never-above-hardlimit-failure branch from d936e3c to 7ba155a Compare November 22, 2021 10:29
@coot
Copy link
Contributor Author

coot commented Nov 22, 2021

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 22, 2021

@iohk-bors iohk-bors bot merged commit 67a7c69 into master Nov 22, 2021
@iohk-bors iohk-bors bot deleted the coot/never-above-hardlimit-failure branch November 22, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connection-manager Issues / PRs related to connection-manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prop_never_above_hardlimit failure
2 participants