Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Define Retry Policy Attributes #977

Merged
merged 11 commits into from
Aug 17, 2022
Merged

Define Retry Policy Attributes #977

merged 11 commits into from
Aug 17, 2022

Conversation

satvu
Copy link
Member

@satvu satvu commented Jul 26, 2022 •
edited
Loading

Issue describing the changes in this PR

#971

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

Additional information

  1. Add FixedDelayRetry and ExponentialBackoffRetry attributes to Worker.Extensions.Abstractions.
  2. Add example usage of attributes to the sample app.
  3. FunctionMetadataGenerator in the SDK has the ability to parse Retry Attributes and write them to functions.metadata JSON file.
  4. FunctionMetadataLoaderExtension is updated to use v1.0.3-preview of Microsoft.Azure.WebJobs.Script.Abstractions so that FunctionMetadata has the Retry property and the JSON Reader can deserialize JObjects from the functions.metadata file into FunctionMetadata as is without code changes.

@satvu satvu changed the title Add retry attributes to function level Define Retry Policy Attributes Jul 26, 2022
@satvu satvu marked this pull request as ready for review July 26, 2022 20:49
@satvu
Copy link
Member Author

satvu commented Jul 26, 2022

Additionally conducted manual verification by running this E2E with the host and inspecting loaded FunctionMetadata to check Retry information was populated correctly. Currently investigating to see what other tests we may need/want for this feature.

kshyju
kshyju reviewed Jul 26, 2022
View reviewed changes
extensions/Worker.Extensions.Abstractions/src/FixedDelayRetryAttribute.cs Show resolved Hide resolved
extensions/Worker.Extensions.Abstractions/src/FixedDelayRetryAttribute.cs Outdated Show resolved Hide resolved
extensions/Worker.Extensions.Abstractions/src/ExponentialBackoffRetryAttribute.cs Show resolved Hide resolved
extensions/Worker.Extensions.Abstractions/src/FixedDelayRetryAttribute.cs Show resolved Hide resolved
extensions/Worker.Extensions.Abstractions/src/ExponentialBackoffRetryAttribute.cs Outdated Show resolved Hide resolved
sdk/FunctionMetadataLoaderExtension/FunctionMetadataLoaderExtension.csproj
@@ -11,7 +11,7 @@

<ItemGroup>
<PackageReference Include="Microsoft.Azure.WebJobs" Version="3.0.25" />
<PackageReference Include="Microsoft.Azure.WebJobs.Script.Abstractions" Version="1.0.0-preview" />
<PackageReference Include="Microsoft.Azure.WebJobs.Script.Abstractions" Version="1.0.3-preview" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 1.0.3 instead of 1.0.1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just grabbed the latest available we had.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see 1.0.0-preview. https://www.nuget.org/packages/Microsoft.Azure.WebJobs.Script.Abstractions/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host uses 1.0.3-preview which is the latest (only published internally). RetryOptions do show up in 1.0.1-preview first, but there's also an update to the RetryOptions class in 1.0.2-preview. Don't know the progress on a public release for these, but I think we can match the host version without issues. If we only want to consume RetryOptions changes we can pick 1.0.2.

sdk/Sdk/Abstractions/SdkRetryOptions.cs Outdated Show resolved Hide resolved
kshyju
kshyju reviewed Aug 2, 2022
View reviewed changes
Copy link
Member

@kshyju kshyju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💰

extensions/Worker.Extensions.Abstractions/src/FixedDelayRetryAttribute.cs Show resolved Hide resolved
extensions/Worker.Extensions.Abstractions/src/FixedDelayRetryAttribute.cs Outdated Show resolved Hide resolved
extensions/Worker.Extensions.Abstractions/src/RetryAttribute.cs Show resolved Hide resolved
samples/FunctionApp/HttpTriggerSimple/HttpTriggerSimple.cs Outdated Show resolved Hide resolved
sdk/FunctionMetadataLoaderExtension/FunctionMetadataLoaderExtension.csproj
@@ -11,7 +11,7 @@

<ItemGroup>
<PackageReference Include="Microsoft.Azure.WebJobs" Version="3.0.25" />
<PackageReference Include="Microsoft.Azure.WebJobs.Script.Abstractions" Version="1.0.0-preview" />
<PackageReference Include="Microsoft.Azure.WebJobs.Script.Abstractions" Version="1.0.3-preview" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see 1.0.0-preview. https://www.nuget.org/packages/Microsoft.Azure.WebJobs.Script.Abstractions/

sdk/Sdk/Abstractions/SdkFunctionMetadata.cs
@@ -23,5 +23,7 @@ internal class SdkFunctionMetadata
public IDictionary<string, object> Properties { get; set; } = new Dictionary<string, object>();

public Collection<ExpandoObject> Bindings { get; set; } = new Collection<ExpandoObject>();

public SdkRetryOptions? Retry { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update the source gen version of metadata generation with this prop as well. Correct? Perhaps a follow up PR later.

extensions/Worker.Extensions.Abstractions/src/ExponentialBackoffRetryAttribute.cs Outdated Show resolved Hide resolved
kshyju
kshyju reviewed Aug 5, 2022
View reviewed changes
Copy link
Member

@kshyju kshyju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

extensions/Worker.Extensions.Abstractions/src/ExponentialBackoffRetryAttribute.cs

private static void ValidateIntervals(TimeSpan minimumInterval, TimeSpan maximumInterval)
{
if (minimumInterval.Ticks < 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What string input(for minimumInterval) will produce a TimeSpan instance with negative value for Ticks property?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"-00:00:10" passes the TimeSpan.Parse check and fails this check.

extensions/Worker.Extensions.Abstractions/src/RetryAttribute.cs Show resolved Hide resolved
extensions/Worker.Extensions.Abstractions/src/RetryAttribute.cs
[AttributeUsage(AttributeTargets.Method)]
public abstract class RetryAttribute : Attribute
{
public RetryAttribute()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this public constructor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent was to stylistically match the other attribute classes (binding attribute, input binding attribute, etc).

samples/FunctionApp/HttpTriggerWithBlobInput/HttpTriggerWithBlobInput.cs Show resolved Hide resolved
sdk/Sdk/FunctionMetadataGenerator.cs Show resolved Hide resolved
test/FunctionMetadataGeneratorTests/FunctionMetadataGeneratorTests.cs Outdated Show resolved Hide resolved
@satvu
Copy link
Member Author

satvu commented Aug 17, 2022

/check-enforcer evaluate

@satvu
Copy link
Member Author

satvu commented Aug 17, 2022

/check-enforcer evaluate

@satvu
Copy link
Member Author

satvu commented Aug 17, 2022

/check-enforcer override

@satvu satvu merged commit 0c808d7 into main Aug 17, 2022
@satvu satvu deleted the satvu/retry-attributes branch August 17, 2022 00:34
@kshyju kshyju mentioned this pull request Mar 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liliankasem liliankasem liliankasem approved these changes

@kshyju kshyju kshyju approved these changes

@brettsam brettsam Awaiting requested review from brettsam

@fabiocav fabiocav Awaiting requested review from fabiocav

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@satvu @kshyju @liliankasem