Skip to content
This repository has been archived by the owner on Nov 15, 2019. It is now read-only.

parallel attempts to bootstrap #108

Merged
merged 2 commits into from
May 21, 2015
Merged

Conversation

bcndanos
Copy link
Contributor

Here you go!

Review on Reviewable

@maidsafe-highfive
Copy link

Thanks for the pull request, and welcome! The MaidSafe team is excited to review your changes, and you should hear from @ned14 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTOR.md for more information.

@chandraprakash
Copy link
Contributor

Thanks a lot Mike. We will have this reviewed and merged. Great work !!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) to 75.3% when pulling a1910b7 on bcndanos:master into 85c7a05 on maidsafe:master.

@dirvine
Copy link
Contributor

dirvine commented May 15, 2015

r? @chandraprakash

@bcndanos
Copy link
Contributor Author

There's a problem with this PR. Don't merge.

Executing the crust_node example

Listening for new connections on Tcp(V4(127.0.1.1:53230)), and listening for UDP broadcast on port 9999.
thread '

' panicked at 'index out of bounds: the len is 0 but the index is 0', /home/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libcollections/vec.rs:1349
An unknown error occurred

@chandraprakash
Copy link
Contributor

Yeah I noticed that too :-)

@hitman401 hitman401 assigned chandraprakash and unassigned ned14 May 15, 2015
@bcndanos
Copy link
Contributor Author

I think now it's fixed. Sorry! :|

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.56%) to 73.61% when pulling ee365bf on bcndanos:master into 85c7a05 on maidsafe:master.

@chandraprakash
Copy link
Contributor

Hi Mike,
Must say very nice work. I tried your PR and it exposed a bug in Crust code #109. It will be handy if we merge your PR after this bug is fixed.
Thanks a lot for your help.

@dirvine
Copy link
Contributor

dirvine commented May 20, 2015

r? @ned14


Review status: 0 of 3 files reviewed, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

@ned14
Copy link
Contributor

ned14 commented May 21, 2015

The logic seems fine, but I think surely it would be better if each successfully bootstrapped node was used instead of throwing away all after the first success. I'll look into that here locally.


Review status: all files reviewed, all discussions resolved, some commit checks failed.
Reviewed files:

  • Cargo.toml @ r1
  • src/connection_manager.rs @ r2
  • src/lib.rs @ r1

Comments from the review on Reviewable.io

@dirvine
Copy link
Contributor

dirvine commented May 21, 2015

Ah good point, we do not trust nodes at all so upper layers (routing in our case) should really ask a whole bunch of bootstrap nodes to get info for them, prior to them getting on the network. This way upper layers can filter replies and ensure they are happy.
The upper layers may need to identify the bootstrap nodes by some id the upper layer uses as the overlay. This may add complexity as upper layers need to speak individually to each bootstrap node to get this info. So from that perspective if we keep all bootstrap guys and upper layers can negotiate with them all and decide to parallel send info then cool. Then we can request a node is promoted from bootstrap to connection. Upper layers can then also request certain bootstrap nodes be dropped when they are happy to do so.
In summary I vote we keep all bootstrap connections, but tag them as such and allow upper payer to do what it wishes with them for as long as they are there.


Review status: all files reviewed, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

@bcndanos
Copy link
Contributor Author

Review status: all files reviewed, 1 unresolved discussion, some commit checks failed.


src/connection_manager.rs, line 316 [r2] (raw file):
I don't know exactly what you want. But, you can use ' Deferred::vec_to_promise(vec_deferred, ControlFlow::ParallelLimit(15)) ' to execute all the conections instead of the first. In that case, the program will be blocked in this process. Or you can pass as first parameter a number N instead of 1 to stop at the firsts N tasks with Ok. Anyway, the next match has to catch the Err(e), because if there's only one error the return is an Err<Vec<Result<T,E>>>, and you have to iter over the vector to get the OK results.


Comments from the review on Reviewable.io

@ned14
Copy link
Contributor

ned14 commented May 21, 2015

Review status: all files reviewed, 1 unresolved discussion, some commit checks failed.
Reviewed files:

  • Cargo.toml @ r1

Cargo.toml, line 20 [r2] (raw file):
On Windows I'm seeing an error after I merge this pull request:

G:\rust-crust>cargo test
Updating registry https://github.com/rust-lang/crates.io-index
cyclic package dependency: package num_cpus v0.2.5 depends on itself

So this pull request appears to break crust on Windows?


Comments from the review on Reviewable.io

@bcndanos
Copy link
Contributor Author

Review status: all files reviewed, 1 unresolved discussion, some commit checks failed.


Cargo.toml, line 20 [r2] (raw file):
Try ' cargo update'. If it doesn't work, try to delete your 'target' directory.


Comments from the review on Reviewable.io

@ned14
Copy link
Contributor

ned14 commented May 21, 2015

Review status: all files reviewed, 1 unresolved discussion, some commit checks failed.


Cargo.toml, line 20 [r2] (raw file):
Sigh. Yes that worked. Sorry about the noise.


Comments from the review on Reviewable.io

@ned14 ned14 merged commit ee365bf into maidsafe-archive:master May 21, 2015
ned14 added a commit that referenced this pull request May 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants