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

Find specific peer from network #981

Merged
merged 15 commits into from Sep 4, 2020

Conversation

limebell
Copy link
Member

@limebell limebell commented Sep 2, 2020

There was Swarm<T>.FindSpecificPeerAsync() method but it was not precise. This PR makes it to work more precisely.

@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #981 into main will increase coverage by 0.10%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
+ Coverage   88.79%   88.89%   +0.10%     
==========================================
  Files         319      320       +1     
  Lines       28130    28132       +2     
==========================================
+ Hits        24977    25007      +30     
+ Misses       1602     1597       -5     
+ Partials     1551     1528      -23     
Impacted Files Coverage Δ
Libplanet/Net/NetMQTransport.cs 82.37% <ø> (-0.07%) ⬇️
Libplanet/Net/Swarm.cs 86.45% <ø> (-0.47%) ⬇️
Libplanet/Net/Protocols/PingTimeoutException.cs 20.83% <20.83%> (ø)
Libplanet/Net/Protocols/KademliaProtocol.cs 75.77% <69.89%> (+6.23%) ⬆️
Libplanet/Net/Protocols/RoutingTable.cs 87.69% <76.92%> (+0.09%) ⬆️
Libplanet.Tests/Net/Protocols/TestTransport.cs 77.50% <100.00%> (+0.06%) ⬆️
Libplanet.Tests/Net/SwarmTest.cs 95.48% <100.00%> (+0.07%) ⬆️
Libplanet.RocksDBStore/RocksDBStore.cs 95.88% <0.00%> (+0.31%) ⬆️
... and 3 more

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.

Instead of directly referring to the constant Kademlia.FindConcurrency from KademliaProtocol, how about making KademliaProtocol() constructor to take that value?

Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
@@ -708,53 +714,35 @@ private void ReceivePing(Ping ping)
{
await Task.WhenAll(awaitables);
}
catch (AggregateException e)
catch (Exception e)
Copy link
Contributor

Choose a reason for hiding this comment

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

@limebell What kind of exceptions do you expect to be thrown here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually TimeoutException, which occurs when the received peer is not alive actually.

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 made TimeoutException (which is replaced by PingTimeoutException) to remove peer from lookup candidate, which will be covered by SwarmTest.FindSpecificPeerAsyncFail() test.

{
peerFound = closestNeighbors[i];
return peerFound;
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be difficult to have a unit test for this case?

Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
earlbread
earlbread previously approved these changes Sep 2, 2020
dahlia
dahlia previously approved these changes Sep 2, 2020
Libplanet/Net/Swarm.cs Outdated Show resolved Hide resolved
@limebell
Copy link
Member Author

limebell commented Sep 2, 2020

Method implemented in this PR keeps searching even the specific target is found. I will add some commits on this PR to fix the issue. Until then, leave title as WIP.

@limebell limebell changed the title Find specific peer from network [WIP] Find specific peer from network Sep 2, 2020
@limebell limebell dismissed stale reviews from dahlia and earlbread via 3e5a10c September 3, 2020 01:50
@limebell limebell changed the title [WIP] Find specific peer from network Find specific peer from network Sep 3, 2020
@limebell
Copy link
Member Author

limebell commented Sep 3, 2020

Used DFS beforehand, may cause stack overflow. So I changed it to BFS by using concurrent queue and stop immediately when target peer is found. However KademliaProtocol.FindSpecificPeer() became to search only one peer at a time, so I'll increase concurrency in next PR.

Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
dahlia
dahlia previously approved these changes Sep 3, 2020
longfin
longfin previously approved these changes Sep 3, 2020
Libplanet/Net/Protocols/PingTimeoutException.cs Outdated Show resolved Hide resolved
longfin
longfin previously approved these changes Sep 4, 2020
earlbread
earlbread previously approved these changes Sep 4, 2020
@limebell limebell merged commit 0cac8b6 into planetarium:main Sep 4, 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.

None yet

4 participants