-
Notifications
You must be signed in to change notification settings - Fork 850
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
Allow customizing retry behavior for timeout failure #2524
Conversation
service/history/workflow/retry.go
Outdated
if strings.HasPrefix(nrt, common.TimeoutFailureTypePrefix) { | ||
continue | ||
} |
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.
no need for this check.
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.
Removed
common/constants.go
Outdated
// used in retry policy | ||
// the actual failure type will be prefix + enums.TimeoutType.String() | ||
// e.g. "Temporal Timeout: StartToClose" or "Temporal Timeout: Heartbeat" | ||
TimeoutFailureTypePrefix = "Temporal Timeout: " |
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.
Maybe we should avoid space in the prefix?
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.
👍 Yup, this is a programmatic name not a sentence.
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.
Sure. Removed both spaces, and now it becomes TemporalTimeout:
common/constants.go
Outdated
// used in retry policy | ||
// the actual failure type will be prefix + enums.TimeoutType.String() | ||
// e.g. "Temporal Timeout: StartToClose" or "Temporal Timeout: Heartbeat" | ||
TimeoutFailureTypePrefix = "Temporal Timeout: " |
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.
👍 Yup, this is a programmatic name not a sentence.
What changed?
Allow customizing retry behavior for timeout errors
Why?
#2496
How did you test it?
Added unit test
Potential risks
This change is technically breaking as the new format defined for timeout failure may matching user defined application error types. But the chance this will happen is very very low.
Is hotfix candidate?
no