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

Separate Connect/Read timeout setting #969

Closed
goenning opened this issue Aug 1, 2022 · 1 comment · Fixed by #971
Closed

Separate Connect/Read timeout setting #969

goenning opened this issue Aug 1, 2022 · 1 comment · Fixed by #971
Labels
client http issues with the client help wanted Not immediately prioritised, please help!

Comments

@goenning
Copy link
Contributor

goenning commented Aug 1, 2022

Would you like to work on this feature?

maybe

What problem are you trying to solve?

I want to fail fast on unreachable api servers by using a low connect timeout, while keeping the actual response/read timeout fairly high.

Describe the solution you'd like

Connect timeout tends to be much lower than read/response timeouts, so there should be an easy way to configure different values for those, it's unlikely that both timeouts should be the same.

Describe alternatives you've considered

There is a property to set the timeout, but it's not granular enough

https://github.com/kube-rs/kube-rs/blob/79f1d7a54ade86932f10ba82429b48abd4ff82cc/kube-client/src/client/builder.rs#L103-L105

I can copy the Client::try_from(cfg) method and customize it, but it's a fairly big chunk of code to copy just to change a timeout setting. If I copy this piece, I'd need to keep it in sync with upstream kube-rs to get improvements/fixes/etc

Documentation, Adoption, Migration Strategy

deprecate timeout field and expose two new fields connect_timeout and read_timeout, give preference to new fields, while doing a fallback to timeout if set. Remove timeout on 1.0.0

Target crate for feature

kube-client

@clux clux added the client http issues with the client label Aug 1, 2022
@clux
Copy link
Member

clux commented Aug 1, 2022

I think this makes sense, strategy is reasonable to me. There's also a third timeout for write_timeout in TimeoutConnector, but we are not setting it atm which may or may not be great.

Given we are using the same value for both inside kube, then it would make sense to split this into options and also fallback to sensible defaults for these when unset. In particular see if there's values to be stolen from client-go (for any of these 3), we could maybe use those as defaults rather than no-timeouts.

Would be happy to take a PR for splitting it out!

@clux clux added the help wanted Not immediately prioritised, please help! label Aug 1, 2022
@clux clux closed this as completed in #971 Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client http issues with the client help wanted Not immediately prioritised, please help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants