Add Retry annotation #139

TsuyoshiUshio · 2020-11-11T17:33:32Z

Create a PR for retry annotation to discuss the spec. The annotation requires maven plugin's change.
I'll update this PR to spot the correct spec once I figure out.

Current Version of the C# implementation has Typo. I correct the spelling on Java. The C# will fix it soon.
Azure/azure-webjobs-sdk#2637

What should we generate from this annotation?

On the maven plugin side, we need to generate retry strategy part. These are examples.
@Flanker32 Could you have a look and can we start discussion for the change the maven plugin side?

Exponential Backoff

function.json

{
  "scriptFile" : "../fabrikam-functions-1.0-SNAPSHOT.jar",
  "entryPoint" : "com.fabrikam.Function.run",
  "bindings" : [ {
    "type" : "httpTrigger",
    "direction" : "in",
    "name" : "req",
    "methods" : [ "GET" ],
    "authLevel" : "ANONYMOUS"
  }, {
    "type" : "http",
    "direction" : "out",
    "name" : "$return"
  } ],
  "retry": {
    "strategy": "exponentialBackoff",
    "maxRetryCount": 5,
    "minimumInterval": "00:00:02",
    "maximumInterval": "00:01:00"
  }
}

Fixed Delay

{
  "scriptFile" : "../fabrikam-functions-1.0-SNAPSHOT.jar",
  "entryPoint" : "com.fabrikam.Function.run",
  "bindings" : [ {
    "type" : "httpTrigger",
    "direction" : "in",
    "name" : "req",
    "methods" : [ "GET" ],
    "authLevel" : "ANONYMOUS"
  }, {
    "type" : "http",
    "direction" : "out",
    "name" : "$return"
  } ],
  "retry": {
    "strategy": "fixedDelay",
    "maxRetryCount": 4,
    "delayInterval": "00:00:05"
  }
}

Azure Functions error handling and retries

It requires Microsoft.Azure.WebJobs 3.0.23 However, the functions host already release it at this version.
https://github.com/Azure/azure-functions-host/releases/tag/v3.0.14785
For the CoreTools that is used by Maven Plugin:

I tested with this version. It works.

Azure Functions Core Tools
Core Tools Version:       3.0.2996 Commit hash: c54cdc36323e9543ba11fb61dd107616e9022bba
Function Runtime Version: 3.0.14916.0

src/main/java/com/microsoft/azure/functions/annotation/FixedDelayRetry.java

pragnagopa · 2020-11-11T18:22:06Z

No need to block on webjobs sdk typo fix as this PR is not affected by it.

pragnagopa · 2020-11-11T18:25:51Z

Shouldn't we add annotation that matches retry options in host.json : https://github.com/Azure/azure-functions-host/blob/dev/sample/NodeRetry/host.json#L3-L7 ? Also, this might need changes in maven plugin to produce right host.json with class level annotations

Method level annotations are required to produce retry section in function.json : https://github.com/Azure/azure-functions-host/blob/dev/sample/NodeRetry/HttpTrigger-RetryFunctionJson/function.json#L18-L22

amamounelsayed · 2020-11-11T18:39:08Z

@pragnagopa This is is really good point, Thank you so much

pragnagopa

Please update the java-library version as part of this PR. Once the changes are in , publish SNAPSHOT version so that mvn plugin can be updated.

TsuyoshiUshio · 2020-11-11T19:13:16Z

Hi @pragnagopa
I have a question for the class level annotation for host.json. What happens if there is several classes?
I think it might be good let the customers to modify host.json. Am I understand correctly?

pragnagopa · 2020-11-11T19:47:44Z

I have a question for the class level annotation for host.json. What happens if there is several classes?

FunctionTimeout is at host.json as well. Would be good to understand how this is handled in mvn

TsuyoshiUshio · 2020-11-12T02:14:45Z

@pragnagopa
From the maven plugin side, for the host.json, they just generate the initial template, if there is not there.

TsuyoshiUshio · 2020-11-12T02:22:13Z

Hi @pragnagopa , @apawast and @jeffhollan

I have a question. I talked with @Flanker32 and he has an idea. Currently, I separate the annotation into too.
However, he gave us an idea for making it into one annotation since, the function.json has only one configuration. We were discussed what happens if the customer has two retry annotations on a function.

What do you think the idea? Hi @Flanker32 If I miss some context, feel free to add the context or correct what I wrote.

Flanker32 · 2020-11-12T06:12:14Z

@TsuyoshiUshio Thanks a lot for your efforts, really exciting feature!

Here is my thoughts for the annotation, as @TsuyoshiUshio said, it seems one Retry annotation should be enough for both the cases, here is an example

public @interface Retry {
    RetryStrategy strategy();

    int maxRetryCount();

    int delayInterval();

    int maximumInterval();

    static enum RetryStrategy {
        EXPONENTIAL, FIXED;
    }
}

By the way could we pass the retry meta data to trigger? So that users may know how many times the trigger failed and send some telemetry based on this data.

Flanker32 · 2020-11-12T06:14:29Z

I have a question for the class level annotation for host.json. What happens if there is several classes?

FunctionTimeout is at host.json as well. Would be good to understand how this is handled in mvn

maven plugin will not modify the host.json, we will just provide an host.json example with function archetype, here is the link

Flanker32 · 2020-11-12T06:33:40Z

For the function.jsonconfiguration, we defined both minimumInterval and maximumInterval for exponentialBackoff, so what will happen if exponential interval is larger then maximumInterval?

For instance, if set minimumInterval to 2s and maximumInterval to 10s, and the interval for the fourth retry will be 16s which higher than the maximum value, will function host rerun the trigger in 10s or will throw an exception in this situation? Do we need to validate the annotation in tooling side? Besides, is there any value limitation for minimumInterval or maximumInterval

pragnagopa · 2020-11-12T15:58:07Z

Yes, Annotation has to match : #139 (comment)

@Flanker32 -

Thanks for clarifying this

maven plugin will not modify the host.json, we will just provide an host.json example with function archetype, here is the link

@TsuyoshiUshio / @amamounelsayed - I believe we never supported class level annotations. Please open a separate issue. If more users ask for it then we can figure out right design. Please scope this PR to adding method level annotations only.

Do we need to validate the annotation in tooling side

No need to add validation in the tooling as the runtime will show proper validation errors at runtime. Same is true for other questions you asked. Validation is handled by the runtime so no need to duplicate validation logic in the tooling.

pragnagopa · 2020-11-12T15:59:41Z

By the way could we pass the retry meta data to trigger? So that users may know how many times the trigger failed and send some telemetry based on this data.

Currently we do not have this. We are tracking the feature here: Azure/azure-webjobs-sdk#2595

TsuyoshiUshio · 2020-11-12T22:03:23Z

I'm ok for the @Flanker32 idea. What do you think? @jeffhollan or @apawast ? #139 (comment)

pragnagopa · 2020-11-12T22:47:12Z

@TsuyoshiUshio - just to clarify , the annotation needed is the retry options that are exposed in the functions host: https://github.com/Azure/azure-functions-host/blob/dev/sample/NodeRetry/HttpTrigger-RetryFunctionJson/function.json#L18-L22 . WebJobs SDK attributes do not apply here. The change needed is #139 (comment)

TsuyoshiUshio · 2020-11-12T22:50:10Z

Hi @pragnagopa Thanks. I also tested from my side by changing function.json manually and it works. So that the annotation definition on the Java side doesn't matter. function.josn matters. So that I'd like to ask PM if it is OK as the customer experience. I'm ok for it.

pragnagopa · 2020-11-12T22:58:05Z

So that the annotation definition on the Java side doesn't matter. function.josn matters

This is the design for all out-of-proc languages. In Java annotation is used to generate function.json so that users manually do not have to edit function.json

TsuyoshiUshio · 2020-11-12T22:59:27Z

Thank you for sharing the context of the design! So that we can design annotation as we like.

TsuyoshiUshio · 2020-11-12T23:07:48Z

Hi @Flanker32 @amamounelsayed @pragnagopa @apawast
I talked with @jeffhollan and he said that

Looks good to me. I like the approach.

I'd like to take your idea. I'll update this PR soon.

TsuyoshiUshio · 2020-11-13T00:05:38Z

Hi @Flanker32
I update the code and make it one Retry. Could you review the code, please?

Hi @pragnagopa
I update the version to 1.5.0-SNAPSHOT

pragnagopa

Added minor comment. Please wait for ACK from @amamounelsayed . Also, open issue in mvn plugin and link it here to ensure we have a track updates on the mvn plugin before publishing a version without -SNAPSHOT

pom.xml

TsuyoshiUshio · 2020-11-13T00:59:11Z

Hi @Flanker32

Do you know why this CI fails happens? it is cloning the maven plugin and cannot find azure-auth-helper. Do I need package repository?

https://github.com/microsoft/azure-maven-plugins/blob/develop/azure-maven-plugin-lib/pom.xml#L116

Flanker32 · 2020-11-13T02:47:18Z

src/main/java/com/microsoft/azure/functions/annotation/Retry.java

+     * The minimum delay interval.
+     * @return The minimum retry delay.
+     */
+    String minimumInterval() default "";


Shall we use integer here and set the interval in second directly? which may be more clear and no need to care about the format. Otherwise, do we support time string like 00:00:120?

I think the format is 00:00:10 . https://docs.microsoft.com/en-us/azure/azure-functions/functions-bindings-error-pages?tabs=java

thats a good point! minimumInterval, maximumInterval and delayInterval get converted to type TimeSpan , is there an equivalent in Java?

Not sure if converting Duration to string will give us the format hh:mm:ss

We can do it. There is an api to fetch it as second. However, I'm not sure Duration is good fit for the annotation. Probably String (as is) or int (second) might be a good fit for it. (I just reply the TimeSpan equivalent) https://www.dariawan.com/tutorials/java/java-time-duration-tutorial-examples/
Hi @Flanker32 Which would you want to prefer? If you choose int, you need to format String. If you use String, no need to convert it since it is the format of function.json IMO, String might be better. the reason is, Functions Host currently support String with that format. In the future, if people add expression on the String on the functions host side, we need to do breaking change. What do you think?

pragnagopa

Sorry for going back and forth on this. Thinking about this more, initial commit you had probably makes sense as the scope is method level only and having separate annotations eliminates guess work for users on what values to provide for each strategy.

This would be similar to what webjobs SDK exposes but mvn would have to generate retry section in function.json

src/main/java/com/microsoft/azure/functions/annotation/RetryStrategy.java

pragnagopa · 2020-11-13T16:44:42Z

src/main/java/com/microsoft/azure/functions/annotation/Retry.java

+     * The minimum delay interval.
+     * @return The minimum retry delay.
+     */
+    String minimumInterval() default "";


Not sure if converting Duration to string will give us the format hh:mm:ss

src/main/java/com/microsoft/azure/functions/annotation/Retry.java

TsuyoshiUshio · 2020-11-16T17:58:12Z

Hi @Flanker32

For the function.jsonconfiguration, we defined both minimumInterval and maximumInterval for exponentialBackoff, so what will happen if exponential interval is larger then maximumInterval?

For instance, if set minimumInterval to 2s and maximumInterval to 10s, and the interval for the fourth retry will be 16s which higher than the maximum value, will function host rerun the trigger in 10s or will throw an exception in this situation? Do we need to validate the annotation in tooling side? Besides, is there any value limitation for minimumInterval or maximumInterval

In this case, the retry duration is 10s. There is no exception. If the retry duration reach to the maximum interval, it keeps the maximum interval.

https://github.com/Azure/azure-webjobs-sdk/blob/b798412ad74ba97cf2d85487ae8479f277bdd85c/src/Microsoft.Azure.WebJobs.Host/Timers/RandomizedExponentialBackoffStrategy.cs

src/main/java/com/microsoft/azure/functions/annotation/FixedDelayRetry.java

src/main/java/com/microsoft/azure/functions/annotation/ExponentialBackoffRetry.java

pragnagopa

Signing off again :) thank you!

Add Retry annotation

a388c98

TsuyoshiUshio requested a review from amamounelsayed as a code owner November 11, 2020 17:33

amamounelsayed reviewed Nov 11, 2020

View reviewed changes

src/main/java/com/microsoft/azure/functions/annotation/FixedDelayRetry.java Outdated Show resolved Hide resolved

pragnagopa requested changes Nov 11, 2020

View reviewed changes

Update ElementType as method

699d819

TsuyoshiUshio added 2 commits November 12, 2020 16:02

Upgrade the version and create Retry annotation and strategy

c962e21

modify the version

de23691

TsuyoshiUshio requested a review from Flanker32 November 13, 2020 00:08

pragnagopa approved these changes Nov 13, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

adjst the version

76438e0

fix ci failure

7fa36fe

Flanker32 reviewed Nov 13, 2020

View reviewed changes

andxu approved these changes Nov 13, 2020

View reviewed changes

pragnagopa reviewed Nov 13, 2020

View reviewed changes

update the strategy name

47786f8

amamounelsayed reviewed Nov 16, 2020

View reviewed changes

src/main/java/com/microsoft/azure/functions/annotation/Retry.java Outdated Show resolved Hide resolved

Separate Retry annotation into two

b9d8619

pragnagopa reviewed Nov 16, 2020

View reviewed changes

src/main/java/com/microsoft/azure/functions/annotation/FixedDelayRetry.java Show resolved Hide resolved

pragnagopa reviewed Nov 16, 2020

View reviewed changes

src/main/java/com/microsoft/azure/functions/annotation/ExponentialBackoffRetry.java Show resolved Hide resolved

Adding strategy

35d16ab

pragnagopa approved these changes Nov 16, 2020

View reviewed changes

amamounelsayed approved these changes Nov 16, 2020

View reviewed changes

Flanker32 approved these changes Nov 17, 2020

View reviewed changes

TsuyoshiUshio merged commit 0098603 into dev Nov 17, 2020

TsuyoshiUshio changed the title ~~[DoNotMerge] Add Retry annotation~~ Add Retry annotation Nov 17, 2020

Flanker32 mentioned this pull request Dec 27, 2020

Support retry annotation in functions maven plugin microsoft/azure-maven-plugins#1203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Retry annotation #139

Add Retry annotation #139

TsuyoshiUshio commented Nov 11, 2020 •

edited

Loading

pragnagopa commented Nov 11, 2020

pragnagopa commented Nov 11, 2020

amamounelsayed commented Nov 11, 2020

pragnagopa left a comment

TsuyoshiUshio commented Nov 11, 2020

pragnagopa commented Nov 11, 2020

TsuyoshiUshio commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020 •

edited

Loading

Flanker32 commented Nov 12, 2020 •

edited

Loading

Flanker32 commented Nov 12, 2020

Flanker32 commented Nov 12, 2020 •

edited

Loading

pragnagopa commented Nov 12, 2020

pragnagopa commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020

pragnagopa commented Nov 12, 2020 •

edited

Loading

TsuyoshiUshio commented Nov 12, 2020

pragnagopa commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020

TsuyoshiUshio commented Nov 13, 2020

pragnagopa left a comment

TsuyoshiUshio commented Nov 13, 2020

Flanker32 Nov 13, 2020

TsuyoshiUshio Nov 13, 2020

pragnagopa Nov 13, 2020

TsuyoshiUshio Nov 13, 2020

pragnagopa Nov 13, 2020

TsuyoshiUshio Nov 13, 2020 •

edited

Loading

pragnagopa left a comment

pragnagopa Nov 13, 2020

TsuyoshiUshio commented Nov 16, 2020

pragnagopa left a comment

Add Retry annotation #139

Add Retry annotation #139

Conversation

TsuyoshiUshio commented Nov 11, 2020 • edited Loading

What should we generate from this annotation?

Exponential Backoff

Fixed Delay

pragnagopa commented Nov 11, 2020

pragnagopa commented Nov 11, 2020

amamounelsayed commented Nov 11, 2020

pragnagopa left a comment

Choose a reason for hiding this comment

TsuyoshiUshio commented Nov 11, 2020

pragnagopa commented Nov 11, 2020

TsuyoshiUshio commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020 • edited Loading

Flanker32 commented Nov 12, 2020 • edited Loading

Flanker32 commented Nov 12, 2020

Flanker32 commented Nov 12, 2020 • edited Loading

pragnagopa commented Nov 12, 2020

pragnagopa commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020

pragnagopa commented Nov 12, 2020 • edited Loading

TsuyoshiUshio commented Nov 12, 2020

pragnagopa commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020

TsuyoshiUshio commented Nov 12, 2020

TsuyoshiUshio commented Nov 13, 2020

pragnagopa left a comment

Choose a reason for hiding this comment

TsuyoshiUshio commented Nov 13, 2020

Flanker32 Nov 13, 2020

Choose a reason for hiding this comment

TsuyoshiUshio Nov 13, 2020

Choose a reason for hiding this comment

pragnagopa Nov 13, 2020

Choose a reason for hiding this comment

TsuyoshiUshio Nov 13, 2020

Choose a reason for hiding this comment

pragnagopa Nov 13, 2020

Choose a reason for hiding this comment

TsuyoshiUshio Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

pragnagopa left a comment

Choose a reason for hiding this comment

pragnagopa Nov 13, 2020

Choose a reason for hiding this comment

TsuyoshiUshio commented Nov 16, 2020

pragnagopa left a comment

Choose a reason for hiding this comment

TsuyoshiUshio commented Nov 11, 2020 •

edited

Loading

TsuyoshiUshio commented Nov 12, 2020 •

edited

Loading

Flanker32 commented Nov 12, 2020 •

edited

Loading

Flanker32 commented Nov 12, 2020 •

edited

Loading

pragnagopa commented Nov 12, 2020 •

edited

Loading

TsuyoshiUshio Nov 13, 2020 •

edited

Loading