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

Expose transport retries as connect_retries in client options #2517

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Dec 28, 2022

Opening this draft of previously discussed addition of an option to set connect retries on the client.

Next steps:

  • Pass type checking in CI
  • Agree on a testing strategy
  • Pass tests in CI

@zanieb
Copy link
Contributor Author

zanieb commented Dec 29, 2022

See encode/httpcore#643 to resolve type errors (outside of tests) by adding retries to SOCKS proxy classes.

@tomchristie
Copy link
Member

Ah neat.

So...

I don't think we need test cases where what we're doing is passing through an argument, and then inspecting the private implementation details to see that it's been set. It should be enough that we have type-checking throughout. The retries API is provided by httpcore, and is tested by httpcore. Separation of concerns etc.etc.

I wonder if we should be avoiding too much top-level configuration API on the client. Perhaps retries or connection_retries should be in the Limits(...) config?

@zanieb
Copy link
Contributor Author

zanieb commented Jan 3, 2023

I don't think we need test cases where what we're doing is passing through an argument, and then inspecting the private implementation details to see that it's been set.

I agree this is really awkward — however, since the behavior differs if you provide your own transport I feel we need a little bit of test coverage? I'm definitely okay with whatever you're comfortable with. Perhaps we should throw an error if you provide a value for this and a transport.

I wonder if we should be avoiding too much top-level configuration API on the client. Perhaps retries or connection_retries should be in the Limits(...) config?

I agree it'd be nice to keep things out of the top-level. Limits feels like a weird place though — everything there is non-zero by default. I don't feel like this setting is adjusting a reasonable upper bound (as limits imply), it's enabling behavior.

@tomchristie tomchristie added enhancement New feature or request discussion labels Jan 4, 2023
@zanieb
Copy link
Contributor Author

zanieb commented Apr 5, 2023

My need for this is not pressing as I've implemented wrapper around the client but I'd love to keep discussing the best way to handle this when you have time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants