Deepomatic / deepomatic-client-python

Python Client
Other
14 stars 0 forks source link

Retry http errors #58

Closed maingoh closed 4 years ago

maingoh commented 5 years ago

Related to #17

maingoh commented 5 years ago
maingoh commented 5 years ago

I dont' understand the problem. The retry is already located in make_request. No need for the developer to change anything. Doing it at the urllib3 level was also considered but doesn't provide as much flexibility as tenacity does (see with @thomas-riccardi). We are also not sure it retries on everything we want.

Overriding the retry parameter from each endpoint is a feature we can probably add. Though for now I don't see a use case where it would be necessary ? This can be done in another PR if really needed one day, but I don't think it provides a lot of value to do it now.

vdel commented 5 years ago

I would rather implement the retry at the low-level in make_request, forwarding retry parameters from the generic retrieve, list, etc... mixups in order to keep the possibility to locally change the retry behavior.

I was referring to places like this one: https://github.com/Deepomatic/deepomatic-client-python/pull/58/files#diff-e63474de88f64ad351a531418838acc4R126 but I didn't see that the retry condition was different from a failure. So actually this comment above is void and everything looks good for me.

maingoh commented 5 years ago

I think I understand what you meant. I could have merged the retry policy of the task with the http helper retry policy instead of using two different retryer, but I don't know if it really make sense to merge the network failures with the retry on "pending" tasks. I don't think it is a big deal though ?

thomas-riccardi commented 5 years ago

I think I understand what you meant. I could have merged the retry policy of the task with the http helper retry policy instead of using two different retryer, but I don't know if it really make sense to merge the network failures with the retry on "pending" tasks. I don't think it is a big deal though ?

Let's discuss it IRL; I am not sure what we want to do, what should be avoided (double level retry?)...

vdel commented 5 years ago

There is retry in case of error 5XX for example, then we use retry to poll on the task until it completes. For me this is okay, it's an important feature so I'm in favor of taking a decision soonish to merge it.