Add minimal example #2062
Add minimal example #2062
Conversation
examples/minimal.rs
Outdated
| @@ -0,0 +1,384 @@ | |||
| // Copyright 2018 MaidSafe.net limited. | |||
| ip: Option<IpAddr>, | ||
| base_port: Option<u16>, | ||
| ) { | ||
| let mut join_handles = Vec::with_capacity(count); |
There was a problem hiding this comment.
why need to have this join_handles, is it will be dropped when return from this function ?
There was a problem hiding this comment.
They join the threads when dropped (see the ScopedJoinHandle wrapper at the bottom of the file). But maybe we don't really need that?
There was a problem hiding this comment.
maybe it's useful later on, for supporting exchanging messages.
However, I don't see it's necessary currently.
There was a problem hiding this comment.
Actually I just tried it. We do need to collect the handles to keep the example running. Without it all the threads are killed immediately and the example terminates.
| init_log(opts.verbosity); | ||
|
|
||
| if opts.count <= 1 { | ||
| start_single_node(opts.first, opts.bootstrap_contacts, opts.ip, opts.port) |
There was a problem hiding this comment.
wondering why need this start_single_node?
can spawn_first_node be used directly?
Also, maybe just call start_multiple_nodes shall be enough? or do we really want a single node example?
There was a problem hiding this comment.
You might want to start multiple instances of this example even across multiple machines, so the ability to start a single node is useful. Also "first node" isn't just the first node started in one instance of the example. It means the first node of the network. So you might want to start first node on one machine and normal node on another. That gives us the following options, all of which I think are valid:
- Single first node
- Single normal node
- Multiple nodes where exactly one of them is first
- Multiple nodes, all normal
examples/minimal.rs
Outdated
| loop { | ||
| // We need receive from multiple channels. As a minimum, we need to receive from the | ||
| // channels used internally by `Node` and from the node event channel. Additionally we might | ||
| // wand to receive from the client network event channel (not used in this example) or from |
| const TARGET_SELF: &str = "minimal"; | ||
| const TARGET_ROUTING: &str = "routing"; | ||
|
|
||
| fn init_log(verbosity: u8) { |
There was a problem hiding this comment.
Would https://github.com/rust-cli/clap-log-flag perhaps make it easier? Our CLI etc. uses this so it would be more consistent?
There was a problem hiding this comment.
Perhaps. But the logging logic I'm using currently is bit different. You can use -v, -vv and -vvv to enable logs (info, debug and trace respectively) from the routing crate only and -vvvv to enable logs from all crates. Doesn't seem I can achieve that with clap-log-flag which allows either only the current crate or all crates.
Also not sure such a simple feature justifies adding yet another dependency.
| @@ -218,7 +217,7 @@ impl Chain { | |||
| Err(InsertError::ReplacedAlreadyInserted) => { | |||
| // TODO: If detecting duplicate vote from peer, penalise. | |||
| log_or_panic!( | |||
There was a problem hiding this comment.
I am not sure log_or_panic is valid anywhere except POC work, but less so here. As this is an example we can log errors or panic, but I don't think mixing them is good. In the cases where it seems to log is fine, unless we get into a UB state.
There was a problem hiding this comment.
We still use log_or_panic in a bunch of places and this PR isn't really about that. I'm OK with replacing log_or_panic with something better, but it should be done as a separate task. We also haven't fully decided what that better thing should be.
There was a problem hiding this comment.
Yes, I agree, we will get to it soon enough. Thanks chap
Closes #2057