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

Add latency and bandwidth options to mocknet #1431

Closed
wants to merge 1 commit into from

Conversation

kbala444
Copy link
Contributor

This PR adds an optional bandwidth cap and latency to the mocknet.

Bandwidth is limited using a ratelimiter object. It uses the token bucket algorithm to determine how long to wait before sending data. Latency and rate limiting are applied on stream writes.

I use the existing LinkOptions struct for configuration.

@GitCop
Copy link

GitCop commented Jun 29, 2015

There were the following issues with your Pull Request

  • Commit: 2a87a64
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>
  • Commit: 6724bc7
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>
  • Commit: 54dbbc6
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>
  • Commit: 073b9f0
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue


This message was auto-generated by https://gitcop.com

@jbenet jbenet added the backlog label Jun 29, 2015
@@ -460,7 +462,7 @@ func TestAdding(t *testing.T) {
if h1 == nil {
t.Fatalf("no network for %s", p1)
}

Copy link
Member

Choose a reason for hiding this comment

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

you'll want to make sure to run go fmt -- http://tip.golang.org/misc/git/pre-commit

@jbenet
Copy link
Member

jbenet commented Jun 30, 2015

@Heems let's drop the extra "remove junk" commits. will want to just "squash" them into the first.

take a look at: https://github.com/ipfs/community/blob/master/docs/amending-commits.md and spend a bit of time playing with git rebase so that you can learn how to squash and amend commits

@jbenet
Copy link
Member

jbenet commented Jun 30, 2015

@whyrusleeping want to CR this?

@kbala444
Copy link
Contributor Author

Right, sorry. I squashed the extra commits, added the signoff, and ran go fmt.

@whyrusleeping
Copy link
Member

@Heems did you have issues trying out my feat/netsim branch? If so, i can help out on that end. Doing latency this way is going to cause headaches

@kbala444
Copy link
Contributor Author

We decided that it would be faster/easier to do it this way for now, since it'd probably take me a lot longer to understand how netsim and mocknet work and figure out what's needed to use netsim as a testnet and control which peers can communicate. The plan was to add realism as we need it, but if this way of adding latency won't work, then I can definitely try to use netsim.

What is the problem with this approach? Would it be enough to incorporate the logic from netsim's transport() method into mocknet or is this just like the completely wrong way of going about latency?

If it isn't salvagable, is netsim at the point where I would just be able to wrap it in its on testnet struct (like an alternate peernet) and be able to use it with bitswap? One specific question I had was the usage of the HostOption() method, if I want to get the host of a peer, do I just make a new peerstore and pass it in there? Are peerstores per peer or per network? Would I have to manage it outside of netsim?

Thanks

@whyrusleeping
Copy link
Member

netsim was designed as a drop-in replacement for the current mocknet. It provides a very similar interface and the same functionality. I'll rebase and show you how it would integrate into your PR.

@whyrusleeping
Copy link
Member

although, if you want, you could just integrate the transport logic into mocknet. The other functionality that netsim adds is the ability to dial peers over the 'network' by their peer ID

@jbenet
Copy link
Member

jbenet commented Jul 1, 2015

@whyrusleeping i wonder if it makes sense to break out mocknet into its own package.

@whyrusleeping
Copy link
Member

@jbenet i like the idea, although it might be difficult to do without also breaking out all of our networking layer into a separate package(s)

@whyrusleeping
Copy link
Member

@Heems alright, i pulled down your bssim code, and it looks like you could integrate netsim into it by replacing your usage of mocknet in createTestNetwork and genInstances. You can also just port the transport code from netsim to mocknet, i'm not sure which route would be the easiest, but netsim was made to replace all usage of mocknet.

@jbenet
Copy link
Member

jbenet commented Jul 1, 2015

@Heems give it a try, if it's not too straightforward, we can always punt, handle the more interesting stuff first, and circle back later.

@kbala444 kbala444 reopened this Jul 6, 2015
@GitCop
Copy link

GitCop commented Jul 6, 2015

There were the following issues with your Pull Request

  • Commit: 791e71b
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue


This message was auto-generated by https://gitcop.com

1 similar comment
@GitCop
Copy link

GitCop commented Jul 6, 2015

There were the following issues with your Pull Request

  • Commit: 791e71b
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue


This message was auto-generated by https://gitcop.com

@jbenet jbenet removed the status/in-progress In progress label Jul 6, 2015
@kbala444 kbala444 closed this Jul 6, 2015
@GitCop
Copy link

GitCop commented Jul 6, 2015

There were the following issues with your Pull Request

  • Commit: 791e71b
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue


This message was auto-generated by https://gitcop.com

@jbenet
Copy link
Member

jbenet commented Jul 6, 2015

@Heems closed?

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