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

re-use reqwest::Client in disk_manual_import #5750

Merged
merged 1 commit into from
May 14, 2024
Merged

Conversation

iliana
Copy link
Contributor

@iliana iliana commented May 13, 2024

Fixes #5717. @faithanalog will follow-up with performance numbers she's running now.

We should probably do this to other clients we generate on-the-fly in Nexus, too, but this is probably the worst offender to fix.

@faithanalog
Copy link
Contributor

Here's a look at things uploading 64 disks in parallel from dogfood -> london

In aggregate I'm seeing a pretty stable 1.1 to 1.2gbit/s on my dogfood VM

saturating the link

On london, nexus is only using 5-6% CPU. previously it was using 100% of a sled's CPU (for a fraction of the upload throughput)

nexus_6pct_2

Interesting to note that since we are processing more requests now, cockroach CPU usage is a bit higher on one of the other sleds. I think this is probably auth handling? Doesn't seem to be a problem at the moment.

cockroachforsomereason

Basically we are uploading like 10x faster while using 1/20th the CPU, and probably not bottlenecking in this part of the process anymore. You love a +18 -4 that makes the thing be good.

@iliana iliana requested review from jgallagher and smklein May 14, 2024 02:36
));
let client = crucible_pantry_client::Client::new_with_client(
&format!("http://{}", endpoint),
self.reqwest_client.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh sorry, I definitely misunderstood this fix when I suggested it sounded like we needed connection pooling. Connection pooling presumably would also do something like this, but seems largely orthogonal to "don't create a million reqwest::Clients", as you pointed out!

@iliana iliana merged commit 8c38ad1 into main May 14, 2024
20 checks passed
@iliana iliana deleted the iliana/reqwest-reuse branch May 14, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nexus does not re-use CruciblePantryClient during disk imports.
3 participants