-
Notifications
You must be signed in to change notification settings - Fork 208
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
Use http/2 and keepalives #1378
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ use std::str::FromStr; | |
use std::sync::atomic::{AtomicBool, AtomicI16, AtomicU8, Ordering}; | ||
use std::sync::Arc; | ||
use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; | ||
use std::{fmt, thread}; | ||
use std::{env, fmt, thread}; | ||
|
||
use log::{max_level, Level}; | ||
|
||
|
@@ -607,11 +607,19 @@ impl Connection { | |
} else { | ||
None | ||
}; | ||
// get pool size from envvar | ||
let pool_max_idle_per_host = match env::var("REGISTRY_CLIENT_POOL_MAX_IDLE_PER_HOST") { | ||
Ok(val) => val.parse::<usize>().unwrap_or(20), | ||
Err(_) => 20, | ||
}; | ||
|
||
let mut cb = Client::builder() | ||
.timeout(timeout) | ||
.connect_timeout(connect_timeout) | ||
.redirect(Policy::none()); | ||
.redirect(Policy::none()) | ||
.use_rustls_tls() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's required for http/2, as far as I know native-tls does not provide for the ALPN required for http/2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would this be acceptable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be acceptable I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I make any other changes to this then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about making the env |
||
.tcp_keepalive(Some(Duration::from_secs(5 * 60))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be configurable. |
||
.pool_max_idle_per_host(pool_max_idle_per_host); | ||
|
||
if config.skip_verify { | ||
cb = cb.danger_accept_invalid_certs(true); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these idle connections take pressure on the registry server? Especially when the container is started and prefetch work is done. Maybe we can make the value as the one option of ConnectionConfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would make a significant impact for reasonable values. Most servers should close idle connections if it becomes a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making the value as the one option of ConnectionConfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to fetch
REGISTRY_CLIENT_POOL_MAX_IDLE_PER_HOST
from configuration.