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.

Sign up for GitHub

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

Jump to bottom

Improve retry error #106

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Improve retry error #106

wants to merge 8 commits into from

Conversation

maingoh
Copy link
Contributor

@maingoh maingoh commented Nov 14, 2023 •
edited
Loading

  • Overriding __str__ of HTTPRetryError to get a more informative error especially when having a bad status code
  • Removing retry on 500 codes: they are 95% of the time programming errors that will retry until timeout and saturate the backends. Instead all errors that are transient and raised as 500 on backend side should be converted into 502-503.
  • Removed python 2.7 support
  • Bump dev dependencies

Close #96

Before

HTTPRetryError[<Future at 0x7fb778751640 state=finished returned Response>]

After

Last attempt was a Response <status_code=502 method=GET url=https://httpbin.org//status/502>

@maingoh maingoh mentioned this pull request Nov 14, 2023
Closed
thomas-riccardi
thomas-riccardi reviewed Nov 14, 2023
View reviewed changes
Copy link
Contributor

@thomas-riccardi thomas-riccardi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

what about nested-services errors?

deepomatic/api/exceptions.py
class HTTPRetryError(RetryError):
pass
def __str__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: renaming this could break sentry alert rules, need to pre-update those before merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could archive the previous alerts and let it create new ones ?

setup.py
'Programming Language :: Python :: 3.8',
'Programming Language :: Python :: 3.9',
'Programming Language :: Python :: 3.10',
'Programming Language :: Python :: 3.11',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what about python 3.12? shipped 2023-10-02

@vdel vdel changed the base branch from master to drop-2.7 May 28, 2024 16:48
Base automatically changed from drop-2.7 to master May 28, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-riccardi thomas-riccardi thomas-riccardi left review comments

@oquerne oquerne Awaiting requested review from oquerne

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove promise package
3 participants
@maingoh @thomas-riccardi @vdel