Removed expiration from RetryPolicy and added one more workflow timeout#35
Merged
mfateev merged 6 commits intotemporalio:masterfrom Apr 29, 2020
Merged
Removed expiration from RetryPolicy and added one more workflow timeout#35mfateev merged 6 commits intotemporalio:masterfrom
mfateev merged 6 commits intotemporalio:masterfrom
Conversation
…edAsNewEventAttributes
alexshtin
approved these changes
Apr 29, 2020
Contributor
alexshtin
left a comment
alexshtin
left a comment
There was a problem hiding this comment.
And I have to left your favourite feedback here: dots at the end of comment lines are missed :-)
decision/message.proto
Outdated
| int32 startToCloseTimeoutSeconds = 9; | ||
| // Maximum time between successful worker heartbeats. | ||
| int32 heartbeatTimeoutSeconds = 10; | ||
| // Retry parameters. Note that activity is retried by default according to a default retry policy. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Retry parameters. Note that activity is retried by default according to a default retry policy. | |
| // Activity retry policy. Note that activity is retried by default according to a default retry policy. |
| // Maximum time an activity is allowed to execute after a pick up by a worker. | ||
| // This timeout is always retriable. Either this or scheduleToCloseTimeoutSeconds is required. | ||
| int32 startToCloseTimeoutSeconds = 9; | ||
| // Maximum time between successful worker heartbeats. |
Contributor
There was a problem hiding this comment.
I would add required/optional and if optional what is default? The same way as you did for all 3 above.
event/message.proto
Outdated
| string firstExecutionRunId = 17; | ||
| common.RetryPolicy retryPolicy = 18; | ||
| int32 attempt = 19; | ||
| int64 workflowExecutionTimeoutTimestamp = 20; |
Contributor
There was a problem hiding this comment.
Timeout is an interval. TimeoutTimestamp looks weird to me. I believe "expiration" is a better word here. workflowExecutionExpirationTimestamp or even workflowExecutionAbsoluteExpirationTimestamp.
| string identity = 9; | ||
| string requestId = 10; | ||
| common.WorkflowIdReusePolicy workflowIdReusePolicy = 11; | ||
| // Retries up to workflowExecutionTimeout |
Contributor
There was a problem hiding this comment.
Suggested change
| // Retries up to workflowExecutionTimeout | |
| // Retries up to workflowExecutionTimeoutSeconds |
| int32 heartbeatTimeoutSeconds = 16; | ||
| // This is an actual retry policy the service uses. | ||
| // It can be different from the one provided (or not) during activity scheduling | ||
| // as the service can override the provided one with default values. |
Contributor
There was a problem hiding this comment.
Usually it is vice versa: "provided values override default values". I think better explanation why and when service ignores provided values.
mfateev
added a commit
to temporalio/api-go
that referenced
this pull request
Apr 29, 2020
yux0
pushed a commit
that referenced
this pull request
Mar 31, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RetryPolicy.expirationIntervalInSecondsspecified for how long the retry policy should keep retrying. The problem is that activity already hasScheduleToClosetimeout that specifies for how long a workflow is willing to wait for an activity completion. This required extending theScheduleToCloseup toRetryPolicy.expirationIntervalby the server which was extremely confusing for users.For workflows
RetryPolicy.expirationIntervalInSecondslimited the duration of retries. At the same time there was no way to limit workflow duration os a cron workflow or a workflow that keeps calling continue as new.This change:
expirationIntervalInSecondsfromRetryPolicyexecutionStartToCloseTimeoutSecondstoworkflowRunTimeoutSecondsworkflowExecutionTimeoutSecondsFrom now on the server is expected to retry activities up to
ScheduleToClosetimeout.The workflow duration is limited to
workflowExecutionTimeoutSecondsacross all the retries and calls to continue as new. The duration of a single run is limited up toworkflowRunTimeoutSeconds.The other expected server change is that an activity should be given a default retry policy and retried by default unless the
retryPolicy. maximumAttemptsis not set to 1.