Honor --max-peers if --min-peers is not specified #8087

niklasad1 · 2018-03-12T16:29:26Z

Attempt to solve #7576

This PR changes the following:

min and max in cli/mod.rs to Option<u16>
none provided => use defaults
min-peers only => max-peers = max(x, DEFAULT_MAX_PEERS)
max-peers only => min-peers = min(x, DEFAULT_MIN_PEERS)
min-peers > max-peers => ERROR
Added tests for this

I'm not sure if this is the right approach but seemed most straight-forward to me

parity-cla-bot · 2018-03-12T16:29:29Z

It looks like @niklasad1 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

twittner · 2018-03-19T15:32:17Z

Sorry for adding a reviewer, mouse click error.

tomusdrw

Looks good, left a suggestion to simplify the code.

tomusdrw · 2018-03-23T13:42:51Z

parity/configuration.rs

@@ -467,7 +474,13 @@ impl Configuration {
 	}

 	fn min_peers(&self) -> u32 {
-		self.args.arg_peers.unwrap_or(self.args.arg_min_peers) as u32
+		match (self.args.arg_max_peers, self.args.arg_min_peers) {


why not:

self.args.arg_min_peers .or(self.args.arg_max_peers) .unwrap_or(DEFAULT_MIN_PEERS) as u32

?

yepp, that is way cleaner

tomusdrw · 2018-03-23T13:43:14Z

parity/configuration.rs

@@ -455,8 +457,13 @@ impl Configuration {
 	}

 	fn max_peers(&self) -> u32 {
-		let peers = self.args.arg_max_peers as u32;
-		max(self.min_peers(), peers)
+		match (self.args.arg_max_peers, self.args.arg_min_peers) {


self.args.arg_max_peers .or(self.args.arg_min_peers) .unwrap_or(DEFAULT_MAX_PEERS) as u32

tomusdrw · 2018-03-27T09:45:47Z

parity/cli/usage.rs

 				// Skip loading config file if no_config flag is specified
-				if raw_args.flag_no_config {
-					return Ok(raw_args.into_args(Config::default()));
+				else if raw_args.flag_no_config {


This else is fishy, why do we ignore no-config flag if max_peers or min_peers are not provided?

Yes, I also think this else is a mistake.

Yepp, you’re right it should be s/else if/if.

I made a faulty assumption that first if statement catches only errors!

andresilva · 2018-03-27T10:11:22Z

parity/cli/usage.rs

 				// Skip loading config file if no_config flag is specified
-				if raw_args.flag_no_config {
-					return Ok(raw_args.into_args(Config::default()));
+				else if raw_args.flag_no_config {


Yes, I also think this else is a mistake.

andresilva · 2018-03-27T10:15:27Z

parity/configuration.rs

-		max(self.min_peers(), peers)
+		self.args.arg_max_peers
+			.or(self.args.arg_min_peers)
+			.unwrap_or(DEFAULT_MAX_PEERS) as u32


In case max_peers isn't defined can we set it to max(min_peers, DEFAULT_MAX_PEERS)? With the current code if you start parity with just --min-peers 5, max peers will also be set to 5 (I'd expect it to use the default value).

Yepp, I'll buy that

andresilva · 2018-03-27T10:16:02Z

parity/configuration.rs

@@ -467,7 +470,9 @@ impl Configuration {
 	}

 	fn min_peers(&self) -> u32 {
-		self.args.arg_peers.unwrap_or(self.args.arg_min_peers) as u32
+		self.args.arg_min_peers


Same as above but with min(max_peers, DEFAULT_MIN_PEERS).

* Only min specified -> Set min to that value and max to default * Only max specified -> Set min and max to that value * Both specified -> Set min the smallest value and max to the largest value

niklasad1 · 2018-03-27T12:13:06Z

parity/configuration.rs

-		let peers = self.args.arg_max_peers as u32;
-		max(self.min_peers(), peers)
+		self.args.arg_max_peers
+			.or(max(self.args.arg_min_peers, Some(DEFAULT_MAX_PEERS)))


This will always return T as long at least one of the arguments is Some but I kept unwrap_or if somebody reacts if we use raw unwrapping

niklasad1 · 2018-03-27T12:13:41Z

parity/configuration.rs

-		self.args.arg_peers.unwrap_or(self.args.arg_min_peers) as u32
+		self.args.arg_min_peers
+			.or(min(self.args.arg_max_peers, Some(DEFAULT_MIN_PEERS)))
+			.unwrap_or(DEFAULT_MIN_PEERS) as u32
 	}


andresilva

LGTM!

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M2-config 📂 Chain specifications and node configurations. labels Mar 12, 2018

niklasad1 changed the title ~~Honor --max-peers not if --min-peers is not specified~~ Honor --max-peers if not --min-peers is not specified Mar 12, 2018

niklasad1 changed the title ~~Honor --max-peers if not --min-peers is not specified~~ Honor --max-peers if --min-peers is not specified Mar 12, 2018

niklasad1 force-pushed the max-min-peers branch from 7b68b15 to 49ac1f0 Compare March 13, 2018 10:10

twittner requested a review from axelchalon March 19, 2018 15:29

twittner removed the request for review from axelchalon March 19, 2018 15:32

niklasad1 force-pushed the max-min-peers branch 2 times, most recently from 57cdbd9 to b9de9f3 Compare March 19, 2018 20:17

5chdn added this to the 1.11 milestone Mar 20, 2018

tomusdrw reviewed Mar 23, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 23, 2018

niklasad1 force-pushed the max-min-peers branch from b9de9f3 to 211b832 Compare March 23, 2018 15:20

tomusdrw reviewed Mar 27, 2018

View reviewed changes

andresilva suggested changes Mar 27, 2018

View reviewed changes

niklasad1 added 4 commits March 27, 2018 14:09

Change interpretation min and max peers

e8e259e

* Only min specified -> Set min to that value and max to default * Only max specified -> Set min and max to that value * Both specified -> Set min the smallest value and max to the largest value

simplify logic, new ParseError & add tests

044b329

simplify code according to the review comments

25af5b5

address review comments

9d421ab

niklasad1 force-pushed the max-min-peers branch from 211b832 to 9d421ab Compare March 27, 2018 12:10

niklasad1 commented Mar 27, 2018

View reviewed changes

more fine-grained tests

549b3c8

andresilva approved these changes Mar 29, 2018

View reviewed changes

andresilva added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Apr 2, 2018

5chdn merged commit 0a535bf into master Apr 3, 2018

5chdn deleted the max-min-peers branch April 3, 2018 14:52

niklasad1 mentioned this pull request Apr 3, 2018

--max-peers not honored if --min-peers is not specified #7576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor --max-peers if --min-peers is not specified #8087

Honor --max-peers if --min-peers is not specified #8087

niklasad1 commented Mar 12, 2018 •

edited

Loading

parity-cla-bot commented Mar 12, 2018

twittner commented Mar 19, 2018

tomusdrw left a comment

tomusdrw Mar 23, 2018

niklasad1 Mar 23, 2018

tomusdrw Mar 23, 2018

tomusdrw Mar 27, 2018

andresilva Mar 27, 2018

niklasad1 Mar 27, 2018

andresilva Mar 27, 2018

andresilva Mar 27, 2018

niklasad1 Mar 27, 2018 •

edited

Loading

andresilva Mar 27, 2018

niklasad1 Mar 27, 2018

niklasad1 Mar 27, 2018

andresilva left a comment

Honor --max-peers if --min-peers is not specified #8087

Honor --max-peers if --min-peers is not specified #8087

Conversation

niklasad1 commented Mar 12, 2018 • edited Loading

parity-cla-bot commented Mar 12, 2018

twittner commented Mar 19, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

niklasad1 commented Mar 12, 2018 •

edited

Loading

niklasad1 Mar 27, 2018 •

edited

Loading