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

default ws note #10014

Closed
wants to merge 12 commits into from
Closed

default ws note #10014

wants to merge 12 commits into from
+12 −15

Conversation

nuke-web3
Copy link
Contributor

Behavior tripped up a user, adding a note for the command and an internal (only) comment on where this is set.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Oct 12, 2021
@nuke-web3 nuke-web3 self-assigned this Oct 12, 2021
@nuke-web3 nuke-web3 added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. I6-documentation Documentation needs fixing, improving or augmenting. labels Oct 12, 2021
adoerr
adoerr approved these changes Oct 13, 2021
View reviewed changes
client/cli/src/commands/run_cmd.rs Outdated
@@ -124,6 +124,9 @@ pub struct RunCmd {
pub ws_port: Option<u16>,

/// Maximum number of WS RPC server connections.
///
/// Default is 100.
// {this is set in client/rpc-servers/src/lib.rs}
#[structopt(long = "ws-max-connections", value_name = "COUNT")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can;t we do something like

#[structopt(long = "ws-max-connections", value_name = "COUNT", default_value="100")]?

And get rid of sc_rpc_server::WS_MAX_CONNECTION?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would also avoid the (likely) miss of the comment needing to be changed if this constant ever changed. grouped in the same lines here would be much easier to spot 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, using default_value doesn't work with using an Option<>. So this change would have quite a bit of ripple effect.

For now, I gues, the updated comment will have to do.

Copy link
Member

Choose a reason for hiding this comment

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

What ripple effect?

@adoerr
Copy link
Contributor

adoerr commented Oct 14, 2021

Ripple effect being that this has to be changed in quite a view places. We also need a new default value, which I use usize::MAX to be.

@adoerr
Copy link
Contributor

adoerr commented Oct 14, 2021

I have no idea, why the CI thinks this PR needs a Polkadot companion. I have done one, and it comes up empty, i.e. no source code changes required. Only Cargo.toml and Cargo.lock.

So, now what?

@kianenigma
Copy link
Contributor

I think we should stick to one pattern. Now, RPC_MAX_PAYLOAD_DEFAULT has a different pattern where the cli arg is Option and const is hardcoded in the pallet, while this one is the opposite. I am fine with both, but want to assert that they should be consistent. Also, I am adding more to the same pattern in #10013

bkchr
bkchr suggested changes Oct 14, 2021
View reviewed changes
bin/node/cli/benches/transaction_pool.rs Outdated
@@ -81,7 +81,7 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
rpc_http: None,
rpc_ws: None,
rpc_ipc: None,
rpc_ws_max_connections: None,
rpc_ws_max_connections: usize::MAX,
Copy link
Member

Choose a reason for hiding this comment

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

No, please don't use usize::MAX

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be a sensible alternative? Use usize::MIN or rely on the Rust language default?

Copy link
Contributor

Choose a reason for hiding this comment

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

will 1024 be good?

Copy link
Contributor

Choose a reason for hiding this comment

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

The actual default is 100 as implied by the default value for the CLI option.

Regarding the review comment above, we are basically looking for something to use instead of None because we no longer use an Option<>.

bkchr
bkchr reviewed Oct 14, 2021
View reviewed changes
bin/node/cli/benches/transaction_pool.rs Outdated Show resolved Hide resolved
client/cli/src/config.rs
@@ -344,8 +344,8 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
/// Get the RPC websockets maximum connections (`None` if unlimited).
///
/// By default this is `None`.
fn rpc_ws_max_connections(&self) -> Result<Option<usize>> {
Ok(None)
fn rpc_ws_max_connections(&self) -> Result<usize> {
Copy link
Member

Choose a reason for hiding this comment

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

The docs above are incorrect.

client/cli/src/config.rs Outdated Show resolved Hide resolved
test-utils/test-runner/src/utils.rs Outdated Show resolved Hide resolved
client/service/test/src/lib.rs Outdated Show resolved Hide resolved
client/cli/src/commands/run_cmd.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Oct 14, 2021

I think a better solution would have been to keep the Option in the configuration and just remove the option from the CLI

@adoerr
Copy link
Contributor

adoerr commented Oct 14, 2021

Yeah, whole PR actually isn't worth it. I'm fine to just get rid of it and leave things as they are.

@stale
Copy link

stale bot commented Nov 13, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Nov 13, 2021
@bkchr bkchr closed this Nov 13, 2021
@bkchr bkchr deleted the NukeManDan-patch-2 branch November 13, 2021 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GopherJ GopherJ GopherJ left review comments

@adoerr adoerr adoerr approved these changes

@bkchr bkchr bkchr requested changes

Assignees

@nuke-web3 nuke-web3

Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. I6-documentation Documentation needs fixing, improving or augmenting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nuke-web3 @adoerr @kianenigma @bkchr @GopherJ