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

Added search for a specific peer given the address #580

Merged
merged 5 commits into from Dec 24, 2019

Conversation

@RozzaysRed
Copy link
Contributor

RozzaysRed commented Oct 14, 2019

Addresses #570

Let me know if there is a better way to approach this. I initially had it setup as having a target address and a specific address but switched it just having a flag being set to say whether they want a list of neighbors to the target or the BoundPeer for the target address.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 14, 2019

Codecov Report

Merging #580 into master will decrease coverage by 0.11%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master     #580      +/-   ##
==========================================
- Coverage   86.75%   86.63%   -0.12%     
==========================================
  Files         223      223              
  Lines       18916    19077     +161     
==========================================
+ Hits        16410    16528     +118     
- Misses       1341     1370      +29     
- Partials     1165     1179      +14
Impacted Files Coverage Δ
Libplanet/Net/Swarm.cs 83.19% <100%> (+0.04%) ⬆️
Libplanet.Tests/Net/SwarmTest.cs 96.04% <100%> (+0.18%) ⬆️
Libplanet/Net/Protocols/KademliaProtocol.cs 65.71% <58.18%> (+0.45%) ⬆️
Libplanet/Store/DefaultStore.cs 85.53% <0%> (-1.1%) ⬇️
Libplanet.Tests/Net/Protocols/TestSwarm.cs 77.85% <0%> (-0.74%) ⬇️
Copy link
Contributor

limebell left a comment

Thanks for your contribution! There are some points that you should be aware of to deal with this issue.

  • Test case(s) for this method.
  • Relay message passing when find specific peer. (Currently, your implementation request specific peer to some address, if the peer does not contain peer with the address in its table but peer with the address is on the network.)

Please take a look at https://en.wikipedia.org/wiki/Kademlia for more information.

Libplanet/Net/Protocols/RoutingTable.cs Outdated Show resolved Hide resolved
@stale

This comment has been minimized.

Copy link

stale bot commented Oct 31, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 31, 2019
@RozzaysRed

This comment has been minimized.

Copy link
Contributor Author

RozzaysRed commented Oct 31, 2019

not stale

@stale stale bot removed the stale label Oct 31, 2019
@kijun

This comment has been minimized.

Copy link
Member

kijun commented Nov 14, 2019

Hey @RozzaysRed ! Have you had time to look into this issue?

@RozzaysRed

This comment has been minimized.

Copy link
Contributor Author

RozzaysRed commented Nov 14, 2019

@kijun we can close this for now if you would like. I've been trying to find time. If I managed to come up with a solution to it using the recommendations and resources provided by @limebell before someone else is able to resolve it then I can submit another PR?

@kijun

This comment has been minimized.

Copy link
Member

kijun commented Nov 21, 2019

We're happy that you're working on this! Just wanted to make sure that you didn't have any stumble blocks.

@RozzaysRed RozzaysRed changed the title Added search for a specific peer given the address [WIP] Added search for a specific peer given the address Nov 25, 2019
@RozzaysRed RozzaysRed force-pushed the RozzaysRed:FIND_PEER branch 2 times, most recently from 52b7d43 to af72005 Nov 25, 2019
@RozzaysRed RozzaysRed changed the title [WIP] Added search for a specific peer given the address Added search for a specific peer given the address Nov 27, 2019
@RozzaysRed RozzaysRed force-pushed the RozzaysRed:FIND_PEER branch 2 times, most recently from 3ecf733 to 13c7cb8 Nov 27, 2019
@moreal moreal requested a review from limebell Dec 4, 2019
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet.Tests/Net/SwarmTest.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
Copy link
Contributor

limebell left a comment

Thank you for your great work! The behavior of your implementation is accurate. Here are some changes I suggest.

Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
Libplanet/Net/Protocols/KademliaProtocol.cs Outdated Show resolved Hide resolved
@RozzaysRed RozzaysRed force-pushed the RozzaysRed:FIND_PEER branch from 464de0f to d4b5d0c Dec 4, 2019
@longfin

This comment has been minimized.

Copy link
Member

longfin commented Dec 5, 2019

Net/SwarmTest.cs(2501,1): error MEN005: File SwarmTest.cs must be no longer than 2500 lines (now 2550). [/home/vsts/work/1/s/Libplanet.Tests/Libplanet.Tests.csproj]

https://dev.azure.com/planetarium/libplanet/_build/results?buildId=2445&view=logs&jobId=eff0806a-f871-53fb-98ec-4f04cba4fef8&taskId=40d54594-fa31-5411-db1e-dbb432feffa2&lineStart=1274&lineEnd=1275&colStart=1&colEnd=1

Builds seem to fail due to file length of SwarmTest.cs 😢

@RozzaysRed Could you increase the value of MaxFileLines in Menees.Analyzers.Settings.xml?

@RozzaysRed RozzaysRed force-pushed the RozzaysRed:FIND_PEER branch from 81603f5 to 88f2636 Dec 5, 2019
@limebell

This comment has been minimized.

Copy link
Contributor

limebell commented Dec 9, 2019

We added patch to stabilize tests recently. The tests would be less fail if you rebase on our latest master 🙇‍♂

@RozzaysRed RozzaysRed force-pushed the RozzaysRed:FIND_PEER branch from 88f2636 to f5976ed Dec 9, 2019
@moreal

This comment has been minimized.

Copy link
Contributor

moreal commented Dec 24, 2019

@RozzaysRed It seems to need rebase.

RozzaysRed and others added 3 commits Nov 25, 2019
Approach is similar to how FindPeerAsync handles requests.  Grab all the
neighbors in the routing table, make sure they are still on the
network. Iterate through neighbors to see if they have a matching
address.  If not, send another round of findneighbor messages

Resolving Requested Changes
@limebell limebell force-pushed the RozzaysRed:FIND_PEER branch from f5976ed to a00d8fb Dec 24, 2019
@limebell limebell requested review from limebell and moreal Dec 24, 2019
@dahlia
dahlia approved these changes Dec 24, 2019
@moreal
moreal approved these changes Dec 24, 2019
@limebell limebell merged commit e825181 into planetarium:master Dec 24, 2019
19 of 20 checks passed
19 of 20 checks passed
benchmarks (macos-latest)
Details
dist
Details
benchmarks (ubuntu-18.04)
Details
benchmarks (windows-latest)
Details
docs
Details
codecov/patch 71.42% of diff hit (target 86.75%)
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
codecov/project 86.63% (-0.12%) compared to 161081e
Details
license/cla Contributor License Agreement is signed.
Details
planetarium.libplanet Build #20191224.6 had test failures
Details
planetarium.libplanet (Linux_Mono) Linux_Mono succeeded
Details
planetarium.libplanet (Linux_NETCore) Linux_NETCore succeeded
Details
planetarium.libplanet (Windows_Mono) Windows_Mono succeeded
Details
planetarium.libplanet (Windows_NETCore) Windows_NETCore succeeded
Details
planetarium.libplanet (Windows_NETCore_coverage) Windows_NETCore_coverage succeeded
Details
planetarium.libplanet (Windows_NETFramework) Windows_NETFramework succeeded
Details
planetarium.libplanet (macOS_Mono) macOS_Mono succeeded
Details
planetarium.libplanet (macOS_NETCore) macOS_NETCore succeeded
Details
planetarium.libplanet (macOS_Unity) macOS_Unity succeeded
Details
@dahlia

This comment has been minimized.

Copy link
Member

dahlia commented Dec 24, 2019

Thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.