search

labd / commercetools-python-sdk

Commercetools Python SDK
https://commercetools-python-sdk.readthedocs.io/en/latest/
MIT License
17 stars 16 forks source link

allow passing custom HTTP adapter to BaseClient #115

Closed lime-green closed 3 years ago

lime-green commented 3 years ago

This PR allows users of the SDK to initialize Client with a custom http_adapter instance. This is useful for setting appropriate timeouts, different retry configuration, and perhaps even for testing.

Our particular use case for this is for setting timeouts, we've noticed CT can sporadically take 10+ seconds for no good reason, and usually gunicorn will timeout and have to kill the worker. It's much preferable to have the timeout set at the request level so gunicorn can just 500 the request and our users can try again

mvantellingen commented 3 years ago

Awesome, thanks. I would love to switch to httpx in the near future. This also allows us to generate async calls.

Did you test the new SDK methods? Looking for feedback there :-)

lime-green commented 3 years ago

@mvantellingen I think for plugging in a different http client, we would need to inject a http client (right now it's using RefreshingOAuth2Session). So when a new http client is introduced in the future, the adapter argument would likely change as well, since I don't think httpx has an equivalent concept (not too sure). I was also considering just adding an argument for each timeout (connect, read, write), and that way they would stay consistent between http clients. I think that's a better change now that I write it out, let me know if you agree

It's funny, I was looking at httpx over the weekend and I was also thinking we should perhaps move our stuff over, it seems like it will be the "new" requests library since it supports additional features (http/2, async) with a cleaner API. Also the default timeout configuration is sane (5 seconds vs. requests has no timeout!?)

Re: the SDK methods, which commit / version were these added? We haven't updated in a few months but I'm looking to update soon :)

tleguijt commented 3 years ago

LGTM! Will merge this so this can be part of the 14.0.0 release