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

Check exception type to set retryCountsAgainstLimit for alarms #2428

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Check exception type to set retryCountsAgainstLimit for alarms #2428

merged 1 commit into from
Jul 25, 2024

Conversation

jqmmes
Copy link
Contributor

@jqmmes jqmmes commented Jul 23, 2024

If an exception is of type OVERLOADED we set retryCountsAgainstLimites = true for alarms.

@jqmmes jqmmes requested review from a team as code owners July 23, 2024 16:19
MellowYarker
MellowYarker reviewed Jul 23, 2024
View reviewed changes
src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved
src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved
src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved
@jqmmes jqmmes force-pushed the joaquim/treat-overloaded-alarms-as-user-error branch from 06fe06d to ae09c6b Compare July 24, 2024 11:02
@jqmmes
Copy link
Contributor Author

jqmmes commented Jul 24, 2024

I've pushed an update to the PR.
@a-robinson this update now checks if we didn't fail due to broken.outputGateBroken. Not sure if this is enough for us to feel comfortable marking the retry as not internal-error.
@justin-mp I've removed the OR logic for the outputGateBroken as that's now part of the shouldRetryCountsAgainstLimit variable.

@jqmmes jqmmes force-pushed the joaquim/treat-overloaded-alarms-as-user-error branch 4 times, most recently from e7520d9 to 15f39bb Compare July 24, 2024 19:44
@jqmmes
Copy link
Contributor Author

jqmmes commented Jul 24, 2024

Following our discussion today, I've updated the PR adding a new custom exception detail that allow us to know if a specific exception is, or is not, a user error.

CC @justin-mp / @a-robinson

a-robinson
a-robinson approved these changes Jul 25, 2024
View reviewed changes
src/workerd/io/actor-cache.c++ Show resolved Hide resolved
src/workerd/api/global-scope.c++ Outdated Show resolved Hide resolved
@jqmmes jqmmes force-pushed the joaquim/treat-overloaded-alarms-as-user-error branch from 15f39bb to a617546 Compare July 25, 2024 16:05
@jqmmes jqmmes force-pushed the joaquim/treat-overloaded-alarms-as-user-error branch from ad70f0f to 5174d2e Compare July 25, 2024 16:12
@jqmmes jqmmes merged commit 9e3b3c3 into main Jul 25, 2024
9 checks passed
@jqmmes jqmmes deleted the joaquim/treat-overloaded-alarms-as-user-error branch July 25, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MellowYarker MellowYarker MellowYarker left review comments

@a-robinson a-robinson a-robinson approved these changes

@justin-mp justin-mp justin-mp approved these changes

@harrishancock harrishancock Awaiting requested review from harrishancock harrishancock is a code owner automatically assigned from cloudflare/workers-runtime-1

@jp4a50 jp4a50 Awaiting requested review from jp4a50 jp4a50 is a code owner automatically assigned from cloudflare/workers-runtime-1

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

Successfully merging this pull request may close these issues.
4 participants
@jqmmes @a-robinson @justin-mp @MellowYarker