Azure / azure-functions-java-library

Contains annotations for writing Azure Functions in Java
MIT License
42 stars 42 forks source link

Add Retry annotation #139

Closed TsuyoshiUshio closed 3 years ago

TsuyoshiUshio commented 3 years ago

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. https://github.com/Azure/azure-webjobs-sdk/pull/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"
  }
}

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
pragnagopa commented 3 years ago

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

pragnagopa commented 3 years ago

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 commented 3 years ago

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

TsuyoshiUshio commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

TsuyoshiUshio commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Yes, Annotation has to match : https://github.com/Azure/azure-functions-java-library/pull/139#issuecomment-725862200

@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 commented 3 years ago

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: https://github.com/Azure/azure-webjobs-sdk/issues/2595

TsuyoshiUshio commented 3 years ago

I'm ok for the @Flanker32 idea. What do you think? @jeffhollan or @apawast ? https://github.com/Azure/azure-functions-java-library/pull/139#issuecomment-725862200

pragnagopa commented 3 years ago

@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 https://github.com/Azure/azure-functions-java-library/pull/139#issuecomment-725862200

TsuyoshiUshio commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

TsuyoshiUshio commented 3 years ago

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 commented 3 years ago

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

TsuyoshiUshio commented 3 years ago

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?

image

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

TsuyoshiUshio commented 3 years ago

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