Skip to content

Conversation

@martintamare
Copy link

With the latest odoo I start to have a lot of HTTPError 429 Too Many Request.
What do you think of this implementation of an auto-retry ?

@martintamare
Copy link
Author

This is a basic implemention. I will try to add test and glue all the pieces together once I know I'm on the right way ;)

@yajo
Copy link
Member

yajo commented Jun 21, 2022

Don't you think an auto-retry might just increase the problem of too many requests?

@martintamare
Copy link
Author

The patch specifically look for 429 errors and sleep exponentially before retrying, so no I don't think so ;)

@martintamare
Copy link
Author

Thanks for the correction, it's not ready yet thow. I will fetch your changes and add the glue with the root object Odoo

@martintamare
Copy link
Author

martintamare commented Jun 21, 2022

Added glue.
Not sure if I should go kwargs all the way down to the proxy itself. I stopped at the Connector.

@martintamare
Copy link
Author

I would also that setting autoretry to True by default can be good.
At least for saas customers like us who use this library in dozen of scripts ...
What do you think about that ?

Comment on lines +41 to +66
# Default autoretry for .odoo.com (saas) hosts
if 'autoretry' in kwargs:
self._autoretry = kwargs['autoretry']
elif host.endswith('.odoo.com'):
self._autoretry = True
else:
self._autoretry = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly like this kind of magic. But in case it were, then you can default the autoretry arg to None, enabling this magic, insteado of relying in the existence of the kwarg. It would be the same result, but simplify the methods signature.

Making things obvious and predictable is good IMHO.

In any case, this (and the rest of the feature) deserves an entry in the docs.

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some typos

martintamare and others added 5 commits May 20, 2024 13:15
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
Co-authored-by: Jairo Llopis <973709+yajo@users.noreply.github.com>
@martintamare martintamare requested a review from yajo May 20, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants