Should allow the Retry node on any Task?

[LeifLazar]

Hello!

Looking at the spec for Retry and Catch and also at the examples, it seems that Retry nodes are only permitted on Catch blocks.

If that's the case and I didn't miss anything, I'd like to discuss the feasibility of allowing Retry nodes on any Task.

Starting from this example

try-catch-retry-inline

- tryGetPet:
  try:
    - getPet:
        call: http
        with:
          method: get
          endpoint: https://petstore.swagger.io/v2/pet/{petId}
  catch:
    errors:
      with:
        type: https://serverlessworkflow.io/spec/1.0.0/errors/communication
        status: 503
    retry:
      delay:
        seconds: 3
      backoff:
        exponential: {}
      limit:
        attempt:
          count: 5

If we wanted to do two API calls and either handle different errors or simply apply different policies, we'd have to do something like this:

Two Tasks

- tryGetPet:
  try:
    - getPet:
        call: http
        with:
          method: get
          endpoint: https://petstore.swagger.io/v2/pet/{petId}
  catch:
    errors:
      with:
        type: https://serverlessworkflow.io/spec/1.0.0/errors/communication
        status: 503
    retry:
      delay:
        seconds: 3
      backoff:
        exponential: {}
      limit:
        attempt:
          count: 5
- tryGetOtherPet:
  try:
    - getOtherPet:
        call: http
        with:
          method: get
          endpoint: https://petstore.swagger.io/v2/pet/{otherPetId}
  catch:
    errors:
      with:
        type: https://serverlessworkflow.io/spec/1.0.0/errors/communication
        status: 504
    retry:
      delay:
        seconds: 10
      backoff:
        exponential: {}
      limit:
        attempt:
          count: 3

The workflow definition gets polluted and readability takes a hit even on simple examples. Nesting gets out of hand fast in a scenario where we'd want to try 3 tasks, handle 5XX errors on all but also handle 4XX errors on only one of them. We'd need nested try-catch blocks.

A naive proposal to try and solve this would look something like this:

Naive

- getPet:
    call: http
    with:
      method: get
      endpoint: https://petstore.swagger.io/v2/pet/{petId}
    errors:
      with:
        type: https://serverlessworkflow.io/spec/1.0.0/errors/communication
        status: 503
    retry:
      delay:
        seconds: 3
      backoff:
        exponential: {}
      limit:
        attempt:
          count: 5
- getOtherPet:
    call: http
    with:
      method: get
      endpoint: https://petstore.swagger.io/v2/pet/{otherPetId}
    errors:
      with:
        type: https://serverlessworkflow.io/spec/1.0.0/errors/communication
        status: 504
    retry:
      delay:
        seconds: 10
      backoff:
        exponential: {}
      limit:
        attempt:
          count: 3

This still doesn't look ideal, but if we'd allow setting the errors filter as an optional parameter of the Retry node, it would look a lot better. I say optional parameter because it would allow "retry on any error" types of behavior if it's not defined. Not sure if that's valuable or not.

Better

- getPet:
    call: http
    with:
      method: get
      endpoint: https://petstore.swagger.io/v2/pet/{petId}
    retry:
      errors:
        with:
          type: https://serverlessworkflow.io/spec/1.0.0/errors/communication
          status: 503
      delay:
        seconds: 3
      backoff:
        exponential: {}
      limit:
        attempt:
          count: 5
- getOtherPet:
    call: http
    with:
      method: get
      endpoint: https://petstore.swagger.io/v2/pet/{otherPetId}
    retry:
      errors:
        with:
          type: https://serverlessworkflow.io/spec/1.0.0/errors/communication
          status: 504
      delay:
        seconds: 10
      backoff:
        exponential: {}
      limit:
        attempt:
          count: 3

Another route would be to leverage the when and exceptWhen parameters of the Retry block but I didn't find any info on whether those runtime expressions have access to the error object or not. If they do, that would eliminate the need for the errors parameter to exist at Retry level.

Of course, any solution would slightly increase the run-time implementation complexity but it shouldn't be too big of a hit since the same logic would anyway have to be implemented for the try-catch block.

Is this even a valid discussion to have? Are there better ways to achieve this?

Thank you!

[cdavernas]

@LeifLazar thank you very much for your input! You make a good point, and I can see how this could improve clarity.

However, this would be a significant breaking change, so it will require a detailed discussion to evaluate its impact across existing implementations. Looking forward to hearing more perspectives on this.

@ricardozanini @JBBianchi @fjtirado @matthias-pichler What do you guys think?

[fjtirado]

Arent we allowing already nested try catch (which I believe is the solution for applying a global policy to a set of task and a particular behaviour to individual ones)?

[LeifLazar]

@cdavernas I was thinking that this would be an optional addition and it wouldn't break compatibility.

@fjtirado Indeed, that's what I was trying to avoid. That nesting grows super fast. This improvement would not make a huge functional difference but it would help to slim down workflow definitions.

[cdavernas]

Ok, yes, makes sense. I don't know why I saw it as a replacement for the try task... The latter would obviously be still valid, as it can be used to define a resilience policy for multiple tasks.
I like your proposal in this that it makes things leaner and cleaner in many cases. The sole change I'd make is the keyword. As a matter if fact, for the sake of consistency, I'd use catch instead of handle, retry, ...

[fjtirado]

@LeifLazar Ok, got it. Can is you illustrate the two proposals with a "code snippet" of how it looks now and how it will look with the modification? That I guess will help to focus the dicussion

[LeifLazar]

There are such snippets in my original post, but I will also add a more complete and complex "before and after" example that we can discuss on.

[fjtirado]

Sorry, my fault, I did not expand them....

[fjtirado]

Looking at the examples, the main difference I see from current situation is that you are removing the try-catch, isnt it?
Im probably biased, but what is the issue with try - catch?

[fjtirado]

But, on the other hand, I think is interesting to explore the options when you want to retry for a reason different than an error happening. The first question is, does this use case really exist? If so, which is the best way to address it?

[LeifLazar]

Looking at the examples, the main difference I see from current situation is that you are removing the try-catch, isnt it?
Im probably biased, but what is the issue with try - catch?

My proposal is not aiming to get rid of the try-catch but to provide an option to specify resiliency policies on any task, without being forced to use try-catch and its implicit nesting.
The try-catch block can still be used for individual tasks but its main benefit is shown when we want to handle errors on a group of tasks.

A common scenario (at least in my mind) is when we have a group of n tasks in a try - catch block which handles 503 errors, but on one of the tasks we want to further handle 4xx errors and we know that another task is susceptible to 504s. We would have to wrap those tasks in two other try-catch blocks.

To oversimplify a bit:

- try
  - try
    - task 1
  - catch
    - handle 504
  - task 2
  - try
    - task 3
  - catch
    - handle 4xx
  - task 4
- catch
  - handle 503

vs.

- try
  - task 1
    - handle 504
  - task 2
  - task 3
    - handle 4xx
  - task 4
- catch
  - handle 503

I'm thinking that the nesting gets worse with larger and more intricate workflow definitions.

I will spend some time to create a real-life scenario comparison of the two approaches.

[ricardozanini]

I like the shortcut, but this will break the public API we have now for 1.0.0 since every task must have a handle definition for this proposal to be in. It can be done if we bend the rules. But we need more examples, discussions, and a migration path. Ideally, what was written in 1.0.0 should work on 1.1.0 (if we add this, it should at least be a minor patch - we can't do majors so soon IMO).

I haven't looked at the schema yet in detail, but that's my main concern.

[JBBianchi]

I like the idea of simplifying the error handling. What about getting rid of try and allow to catch directly at (any) task level ?

document:
  #...
do:
- myFirstTask:
    do:
    - aSubTask:
      #...
    - anotherSubTask:
        call: http
        with: #...
        catch: #<-- catch on a single task
          errors:
            with:
              status: 503
              #...
          as: error
          retry:
            #...
- mySecondTask:
    call: http
    with: #...
- myThirdTask:
    do:
    - yetAnotherSubTask:
      #...
    - someSubTask:
        call: http
        with: #...
    catch: #<-- catch on a group of tasks
      errors:
        with:
          status: 503
          #...
      as: error
      retry:
        #...
catch: #<-- global catch
  errors:
    with:
      status: 503
      #...
  as: error
  retry:
    #...

This would break backward compatibility though. We could mark try as obsolete for some time before removing it altogether in a future major version for instance.

[matthias-pichler]

I think simplifying error handling is definitely worthwhile. 👍

From a versioning perspective adding an optional catch property on every task should be doable in a minor version change. Removing try...catch altogether seems a bit rushed though as it still is a valuable tool.

So I am in favor of @JBBianchi 's proposal to add catch on all tasks but to keep try

[ricardozanini]

Indeed, we can add an optional catch to 1.1.0 but keep try..catch to avoid breaking compatibility.

Later, we remove try...catch in a major version. I like this idea too.

[JBBianchi]

@LeifLazar I think we all reached a consensus. Do you want to open the related issue and maybe work on the PR?

[LeifLazar]

Yes, I will take this on, thank you!