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

Plumbing to increase pvf workers configuration based on chain id #4252

Merged
merged 4 commits into from
Apr 24, 2024

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Apr 23, 2024

Part of #4126 we want to safely increase the execute_workers_max_num gradually from chain to chain and assess if there are any negative impacts.

This PR performs the necessary plumbing to be able to increase it based on the chain id, it increase the number of execution workers from 2 to 4 on test network but lives kusama and polkadot unchanged until we gather more data.

Part of #4126 we want
to safely increase the execute_workers_max_num gradually from chain to
chain and assess if there are any negative impacts.

This PR performs the necessary plumbing to be able to increase it based
on the chain id, it increase the number of execution workers from 2 to 4
on test network but lives kusama and polkadot unchanged until we gather
more data.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

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

Nice!
As an idea, we could even put them into the executor params if need be.

@alexggh
Copy link
Contributor Author

alexggh commented Apr 23, 2024

As an idea, we could even put them into the executor params if need be.

Yeah, I thought about that as well, the reason I think this makes sense in the node version, is two fold:

  1. No big bang when we change it, since nodes don't upgrade all at once.
  2. On the worse case scenario, if this is bad for a given HW node, we can just tell them to downgrade until we figure it out a different solution.

@sandreim
Copy link
Contributor

I don't think this should be in the ExecutorParams which should contain stuff that has consensus implications.

It might be a good idea to expose these to be overwritten by CLI parameter, so if any issues validators can just keep using current version.

@ordian ordian added T0-node This PR/Issue is related to the topic “node”. T8-polkadot This PR/Issue is related to/affects the Polkadot network. and removed T0-node This PR/Issue is related to the topic “node”. labels Apr 23, 2024
Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6024991

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh
Copy link
Contributor Author

alexggh commented Apr 23, 2024

It might be a good idea to expose these to be overwritten by CLI parameter, so if any issues validators can just keep using current version.

Done, let me know if all good now.

Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

LGTM!

This demands a PRDoc, please add one before merging.

Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
@alexggh alexggh enabled auto-merge April 24, 2024 05:51
@alexggh alexggh added this pull request to the merge queue Apr 24, 2024
Merged via the queue into master with commit 9a0049d Apr 24, 2024
135 of 137 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants