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

BugFix: Abort on certain response codes and retry on others #387

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jdye64
Copy link
Collaborator

@jdye64 jdye64 commented Jan 29, 2025

Description

The previous logic grouped all possible HTTP response codes into the same "bucket". This meant that any response received from the server would result in a retry. Some HTTP response codes like 404 & 500 are unlikely to remediate themselves with a retry with the extra same input and conditions and therefore spam the client with nearly unlimited retries. This PR adjust that logic to instead through RuntimeErrors in those conditions and offer a placeholder where other conditions of that manner can later be added as well.

This closes #382

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

… down into response code and further group errors as ones that should be retried and ones that are hard failures
# There are a plethora of codes that can be thrown. Some offer specific insights while
# others can be grouped into a general failure category. We check for specific codes here
# and then generally error on the others.
if response.response_code == 404:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if we want to make the generic client aware of HTTP response codes. It's also used for Redis/SimpleBroker, and whatever else we may need to support in the future.

Maybe we just have recoverable vs non-recoverable response codes that it knows about and translate errors from the different client instances into those?

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.

[BUG]: Incorrect behavior with REST client's 'fetch_message' API when a job ID does not exist.
2 participants