java-sdk icon indicating copy to clipboard operation
java-sdk copied to clipboard

Add taskName and input to retry context

Open TheForbiddenAi opened this issue 7 months ago • 7 comments

Description

Add taskName and input to WorkflowTaskRetryContext Changed workflowContext type in WorkflowTaskRetryContext to WorkflowContext

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1422

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • [x] Code compiles correctly
  • [x] Created/updated tests
  • [x] Extended the documentation

TheForbiddenAi avatar Jun 13 '25 13:06 TheForbiddenAi

@TheForbiddenAi thank you for the insightful explanation. The PR looks good to me. Thank you

siri-varma avatar Jun 16 '25 21:06 siri-varma

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.49%. Comparing base (d759c53) to head (30a41cd). Report is 172 commits behind head on master.

Files with missing lines Patch % Lines
...va/io/dapr/workflows/WorkflowTaskRetryContext.java 60.00% 2 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1423      +/-   ##
============================================
+ Coverage     76.91%   78.49%   +1.57%     
- Complexity     1592     1867     +275     
============================================
  Files           145      230      +85     
  Lines          4843     5794     +951     
  Branches        562      601      +39     
============================================
+ Hits           3725     4548     +823     
- Misses          821      926     +105     
- Partials        297      320      +23     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jun 16 '25 21:06 codecov[bot]

Workflow failure is not related to these changes. They should pass on rerun

TheForbiddenAi avatar Jun 16 '25 22:06 TheForbiddenAi

@TheForbiddenAi @siri-varma I have some issues with this idea. I believe that the task name needs to be propagated to the runtime to keep track of the task name. Not having this at the runtime level, will make it impossible from the orchestrator (the dapr sidecar) to keep track of what is executed, as this is only being added on the SDK side. This also brings inconsistency with other SDKs.

salaboy avatar Jun 17 '25 05:06 salaboy

@salaboy I'm confused at what you mean by the task name having to be propagated to the runtime. This wouldn't change at. I don't see how this affects the knowledge of the orchestrator either. The orchestrator is what calls the retry handler in the first place. We just wrap the objects to not expose the durable task ones. This is just adding extra information, which we already have available to us when the context objects get instantiated at runtime, to that object.

As far as inconsistencies go, I don't think other SDKs have added support for durabletask's retry handler.

TheForbiddenAi avatar Jun 17 '25 09:06 TheForbiddenAi

What I meant is that the task name is something more general than just the retry mechanism, hence just adding the task name for the retry mechanism feels wrong to me.

  • Blog: http://salaboy.com http://salaboy.wordpress.com

  • Github user: http://github.com/salaboy

  • Twitter: http://twitter.com/salaboy

  • Mauricio "Salaboy" Salatino -

On Tue, 17 Jun 2025 at 18:24, Mason @.***> wrote:

TheForbiddenAi left a comment (dapr/java-sdk#1423) https://github.com/dapr/java-sdk/pull/1423#issuecomment-2979620519

@salaboy https://github.com/salaboy I'm confused at what you mean by the task name having to be propagated to the runtime. This wouldn't change at. I don't see how this affects the knowledge of the orchestrator either. The orchestrator is what calls the retry handler in the first place. We just wrap the objects to not expose the durable task ones. This is just adding extra information, which we already have available to us when the context objects get instantiated at runtime, to that object.

As far as inconsistencies go, I don't think other SDKs have added support for durabletask's retry handler.

— Reply to this email directly, view it on GitHub https://github.com/dapr/java-sdk/pull/1423#issuecomment-2979620519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACCMXQHBGH2RP3YKUOVKOT3D7NGBAVCNFSM6AAAAAB7IGYGQWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNZZGYZDANJRHE . You are receiving this because you were mentioned.Message ID: @.***>

salaboy avatar Jun 18 '25 10:06 salaboy

I still don't really see the problem. You could say the same thing about the orchestrator context that the original durable task retry context passes. I don't see the issue with providing the user more information when retrying to allow for more complex logic. However, if you truly think this is a blocker for this PR I will take out the task name from the context.

TheForbiddenAi avatar Jun 18 '25 12:06 TheForbiddenAi