Support activity retry delay #571
Conversation
temporalio/converter.py
Outdated
| delay = google.protobuf.duration_pb2.Duration() | ||
| delay.FromTimedelta(error.next_retry_delay) | ||
| failure.application_failure_info.next_retry_delay.CopyFrom(delay) |
There was a problem hiding this comment.
| delay = google.protobuf.duration_pb2.Duration() | |
| delay.FromTimedelta(error.next_retry_delay) | |
| failure.application_failure_info.next_retry_delay.CopyFrom(delay) | |
| failure.application_failure_info.next_retry_delay.FromTimedelta(error.next_retry_delay) |
I wonder if this code works instead of a create-then-copy
There was a problem hiding this comment.
Thanks, yes it seems to.
temporalio/exceptions.py
Outdated
| a delay before the next activity retry. Ignored if set when raising | ||
| ApplicationError from workflow code. |
There was a problem hiding this comment.
I don't believe the last sentence is true anymore: temporalio/api#426
|
|
||
|
|
||
| async def test_activity_retry_delay(client: Client): | ||
| retry_delay = timedelta(seconds=2) |
There was a problem hiding this comment.
Will this make the test take 2 seconds? I think we can just check whether the property was accepted by the server and set on the way back (e.g. see https://github.com/temporalio/sdk-dotnet/pull/254/files#diff-bfb9d5dbaaa6ec1dfd5ce1837ad44f89fdb2bb75195af55b207ea9627a521eb6) instead of the actual server behavior
There was a problem hiding this comment.
Yes, I was following what we do in the Typescript test: https://github.com/temporalio/sdk-typescript/blob/daf1dca2805ae644a80469a8486c4d719184efd5/packages/test/src/test-integration-workflows.ts#L404-L436
There was a problem hiding this comment.
I think we can just test whether the exception has this property set instead of testing server behavior. These kinds of tests that have code duration expectations become flaky in slower environments. Here's an example in .NET where we only confirm property is set, not actual server timing: https://github.com/temporalio/sdk-dotnet/blob/4a6d4a0e886360242ea66d3402095b9b4406d690/tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs#L4557. I think we should be able to trust if the server has the property set it will do the right thing.
Fixes #468
Support setting
next_retry_delaywhen raisingApplicationErrorfrom activity code.Evidence that this is correct
PR contains an integration test.