-
Notifications
You must be signed in to change notification settings - Fork 141
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
Enforce concurrency on KBucket
internal dictionaries
#1879
Conversation
643359d
to
f709fa0
Compare
KBucket
internal dictionaries
I guess this is not a flaky test. could you check why this test is broken? 🤔 |
551eb83
to
f9681a4
Compare
@riemannulus Not sure... seems flaky even though it isn't tagged as such. 😶 |
Assert.DoesNotContain(transportB.AsPeer, transport.Peers); | ||
Assert.Contains(transportC.AsPeer, transport.Peers); |
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.
What changes lead to this result?
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.
Additionally, internal replacement cache size is limited to the size of a KBucket, i.e. the total number of Addresses a KBucket can hold up to is twice its "bucket size". This results in a slight behavioral change of a KBucket. See ProtocolTest.cs in the code.
This. 😐
Since the test is performed with a KBucket
of size
1
, only C
remains in ReplacementCache
as B
is overwritten by the later addition of C
.
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.
👌
Count added Head and Tail added Clear added Unused variable cleanup Logger added Peers and PeerStates added Cleanup Missing overload Filename change Method name change for clarity Documentation Minor additional description
Cleanup
f9681a4
to
b6a8240
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Partially addresses #1872. Note that only internal dictionaries are now concurrent, but
KBucket
itself is not.KBucket
needs to orchestrate access to two internal dictionaries. Overall orchestration of the dictionaries will be handled in a different PR while also dealing with #1880.Additionally, internal replacement cache size is limited to the size of a
KBucket
, i.e. the total number ofAddress
es aKBucket
can hold up to is twice its "bucket size". This results in a slight behavioral change of aKBucket
. SeeProtocolTest.cs
in the code.