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
tests/worker/test_activity.py
Outdated
|
|
||
|
|
||
| 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.
dandavison
force-pushed
the
468-activity-retry-delay
branch
from
d91d3b3 to
6eabe69
Compare
tests/worker/test_activity.py
Outdated
|
|
||
|
|
||
| @workflow.defn | ||
| class ActivitiesWithRetryDelayWorkflow: |
There was a problem hiding this comment.
This test file was built for activities only against the kitchen sink workflow before the workflow worker was created in Python. I am not sure it is safe for use in a sandbox like test_workflow.py is and that's probably why CI is failing. I think this test should move into that file that is kept safe for sandbox use.
(Surprising 5ms passed on anything?)
Fixes #468
Support setting
next_retry_delaywhen raisingApplicationErrorfrom activity code.Evidence that this is correct
PR contains an integration test.