-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(taskbroker): Clarify docstring & class name #103893
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
Conversation
Changes included: * A Markdown table is added to the docstring describing how to call the `retry` decorator. * It renames `RetryError` to `RetryTaskError` to help search the codebase.
| The first set of parameters define how different exceptions are handled. | ||
| Raising an error will still report a Sentry event. | ||
| | Parameter | Retry | Report | Raise | Description | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @functools.wraps(func) | ||
| def wrapped(*args, **kwargs): | ||
| task_state = current_task() | ||
| no_retries_remaining = task_state and not task_state.retries_remaining |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting these variables here is to simplify a follow-up PR I have.
| from sentry.taskworker.constants import CompressionType | ||
| from sentry.taskworker.registry import TaskNamespace | ||
| from sentry.taskworker.retry import Retry, RetryError, retry_task | ||
| from sentry.taskworker.retry import Retry, RetryTaskError, retry_task |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming the variable name makes it easier to search for this.
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From here on, there's only renaming of the variable across the codebase.
markstory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I'm not sure that going back to stack grouping for retry errors is the right choice though.
I have addressed your feedback and I'm retracting it. |

Changes included:
retrydecorator.RetryErrortoRetryTaskErrorto help search the codebase.