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

Separate peer discovery from transport #1120

Merged
merged 9 commits into from
Dec 16, 2020

Conversation

limebell
Copy link
Member

@limebell limebell commented Dec 3, 2020

This patch separates peer discovery logic from transport.

Skipped changelog for NetMQTransport constructor because it is added in 0.11.

@limebell limebell self-assigned this Dec 3, 2020
@codecov
Copy link

codecov bot commented Dec 3, 2020

Codecov Report

Merging #1120 (62dc896) into main (11a01b2) will decrease coverage by 0.03%.
The diff coverage is 86.63%.

@@            Coverage Diff             @@
##             main    #1120      +/-   ##
==========================================
- Coverage   87.58%   87.55%   -0.04%     
==========================================
  Files         341      341              
  Lines       29747    29694      -53     
==========================================
- Hits        26055    25999      -56     
- Misses       2057     2065       +8     
+ Partials     1635     1630       -5     
Impacted Files Coverage Δ
Libplanet.Tests/Net/SwarmTest.Fixtures.cs 100.00% <ø> (ø)
Libplanet/Net/Protocols/KademliaProtocol.cs 78.61% <78.66%> (-1.02%) ⬇️
Libplanet/Net/Swarm.cs 83.21% <83.07%> (+0.51%) ⬆️
Libplanet/Net/NetMQTransport.cs 78.32% <83.33%> (-0.35%) ⬇️
Libplanet/Net/Protocols/RoutingTable.cs 89.39% <91.11%> (-2.86%) ⬇️
Libplanet.Tests/Net/Protocols/ProtocolTest.cs 100.00% <100.00%> (ø)
Libplanet.Tests/Net/Protocols/RoutingTableTest.cs 94.01% <100.00%> (-0.48%) ⬇️
Libplanet.Tests/Net/Protocols/TestTransport.cs 74.92% <100.00%> (-0.31%) ⬇️
Libplanet.Tests/Net/SwarmTest.cs 95.81% <100.00%> (+<0.01%) ⬆️
Libplanet/Net/Protocols/Kademlia.cs 93.93% <100.00%> (-0.18%) ⬇️
... and 6 more

@limebell limebell requested review from dahlia, longfin, moreal and riemannulus and removed request for dahlia December 4, 2020 08:07
@limebell limebell force-pushed the refactor/protocol branch 3 times, most recently from 700bd3d to e6e77a6 Compare December 4, 2020 08:14
@limebell limebell marked this pull request as ready for review December 4, 2020 08:30
longfin
longfin previously approved these changes Dec 4, 2020
_requestTimeout =
requestTimeout ??
TimeSpan.FromMilliseconds(Kademlia.IdleRequestTimeout);
TimeSpan.FromMilliseconds(5000);
Copy link
Member

Choose a reason for hiding this comment

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

Is it okay we use 5000 directly instead of named constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it is weird that timeout value is located at Kademlia constants.

Copy link
Contributor

@moreal moreal left a comment

Choose a reason for hiding this comment

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

It seems to need to resolve conflict 🙏

Libplanet/Net/Protocols/Kademlia.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/Kademlia.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/Kademlia.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
public int Count => _buckets.Sum(bucket => bucket.Count);

/// <summary>
/// <see cref="IEnumerable{T}"/> of peers in the table.
Copy link
Contributor

@moreal moreal Dec 7, 2020

Choose a reason for hiding this comment

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

Suggested change
/// <see cref="IEnumerable{T}"/> of peers in the table.
/// A list of peers in the table.

With the XML document, we will see a strange sentence like the below link.

image

https://docs.libplanet.io/main/api/Libplanet.Net.ITransport.html#Libplanet_Net_ITransport_BootstrapAsync_IEnumerable_Libplanet_Net_BoundPeer__System_Nullable_TimeSpan__System_Nullable_TimeSpan__System_Int32_CancellationToken_

Copy link
Contributor

@dahlia dahlia Dec 7, 2020

Choose a reason for hiding this comment

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

@moreal That's a known issue (at least to me), and it's the problem with our build settings with DocFX. (FYI it's properly rendered on my laptop.) We shouldn't remove the <see> element due to this, but we should fix the DocFX configuration instead.

dahlia
dahlia previously approved these changes Dec 8, 2020
longfin
longfin previously approved these changes Dec 10, 2020
moreal
moreal previously approved these changes Dec 10, 2020
Copy link
Contributor

@dahlia dahlia left a comment

Choose a reason for hiding this comment

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

This should be rebased again.

moreal
moreal previously approved these changes Dec 15, 2020
dahlia
dahlia previously approved these changes Dec 15, 2020
longfin
longfin previously approved these changes Dec 16, 2020
@longfin longfin merged commit 892f99a into planetarium:main Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants