Remove final keyword for RetryPolicy so that storage SDK can inherit from it

[Jinming-Hu]

closes #5515

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

• C++ Guidelines
• Doxygen docs
• Unit tests
• No unwanted commits/changes
• Descriptive title/description
  • PR is single purpose
  • Related issue listed
• Comments in source
• No typos
• Update changelog
• Not work-in-progress
• External references or docs updated
• Self review of PR done
• Any breaking changes?

[RickWinter]

@Jinming-Hu Where is this policy added to the pipeline? Is storage planning to replace the default retry policy with this policy?

[Jinming-Hu]

@Jinming-Hu Where is this policy added to the pipeline? Is storage planning to replace the default retry policy with this policy?

Yes, exactly. If core SDK cannot expose an API for storage to add custom logic, we'll have to add our own retry policy.

[ahsonkhan]

we'll have to add our own retry policy.

Where are you planning to use it and hook it up to the HTTP pipeline? I don't see this new StorageRetryPolicy used anywhere in this PR.
What I see is the StoragePerRetryPolicy:

azure-sdk-for-cpp/sdk/storage/azure-storage-queues/src/queue_service_client.cpp

Lines 49 to 53 in e28ac6c

	perRetryPolicies.emplace_back(std::make_unique<_internal::StorageSwitchToSecondaryPolicy>(
	m_serviceUrl.GetHost(), newOptions.SecondaryHostForRetryReads));
	perRetryPolicies.emplace_back(std::make_unique<_internal::StoragePerRetryPolicy>());
	perOperationPolicies.emplace_back(
	std::make_unique<_internal::StorageServiceVersionPolicy>(newOptions.ApiVersion.ToString()));

The HttpPipeline ctor will still use the base RetryPolicy:

azure-sdk-for-cpp/sdk/core/azure-core/inc/azure/core/internal/http/pipeline.hpp

Lines 123 to 125 in e28ac6c

	// Retry policy
	m_policies.emplace_back(std::make_unique<Azure::Core::Http::Policies::_internal::RetryPolicy>(
	clientOptions.Retry));

[Jinming-Hu]

@ahsonkhan I updated the PR with a use case in blob_client.cpp. I'd like to keep this PR simple, so I'll update other use cases in separate PRs.

StoragePerRetryPolicy adds some time-sensitive headers before each retry. It has nothing to do with retry logic itself. let's not pay attention to it.

[Jinming-Hu]

@ahsonkhan can you take another look at this PR?

[RickWinter]

@LarryOsterman , @ahsonkhan

@Jinming-Hu Where is this policy added to the pipeline? Is storage planning to replace the default retry policy with this policy?

Yes, exactly. If core SDK cannot expose an API for storage to add custom logic, we'll have to add our own retry policy.

We need to document our design for retry policy modifications.

[ahsonkhan]

We need to document our design for retry policy modifications.

I was curious to see how other languages extend this capability, since this is a language agnostic feature.

One approach is making the RetryPolicy itself inheritable and expose customization capabilities on logic that can be injected before and after each retry attempt, and have the SDK author customize when to retry.

For example, .NET does this by having virtual/overridable methods including:

• ShouldRetry
• OnSendingRequest
• OnRequestSent

That said, I didn't see many compelling examples of service SDKs overriding the OnSendingRequest / OnRequestSent methods.

The other approach is to add the customization logic to the RetryOptions.

Javas has a ShouldRetryCondition predicate as part of their RetryOptions to customize the logic for when to actually retry a request. This is similar to Go, which has a ShouldRetry predicate func as part of its RetryOptions:
https://github.com/Azure/azure-sdk-for-go/blob/01eb1703eea5f83182942e2865feedda0e162177/sdk/azcore/policy/policy.go#L126-L133

We could do something similar in C++ and add that field in the options.

[Jinming-Hu]

We need to document our design for retry policy modifications.

I was curious to see how other languages extend this capability, since this is a language agnostic feature.

One approach is making the RetryPolicy itself inheritable and expose customization capabilities on logic that can be injected before and after each retry attempt, and have the SDK author customize when to retry.

For example, .NET does this by having virtual/overridable methods including:

• ShouldRetry
• OnSendingRequest
• OnRequestSent

That said, I didn't see many compelling examples of service SDKs overriding the OnSendingRequest / OnRequestSent methods.

The other approach is to add the customization logic to the RetryOptions.

Javas has a ShouldRetryCondition predicate as part of their RetryOptions to customize the logic for when to actually retry a request. This is similar to Go, which has a ShouldRetry predicate func as part of its RetryOptions: https://github.com/Azure/azure-sdk-for-go/blob/01eb1703eea5f83182942e2865feedda0e162177/sdk/azcore/policy/policy.go#L126-L133

We could do something similar in C++ and add that field in the options.

I'm fine with either way. Can we prioritize this? This is blocking STG94 feature work.

[ahsonkhan]

@Jinming-Hu if you have it available, can you share links/info on proof of concepts/PRs by storage SDK owners for how they plan to implement this storage STG94 feature work in other languages? It would be helpful to see how the storage scenario is enabled using existing customization options for Java/.NET/Go/etc. for us to design how C++ should customize the RetryPolicy.

[Jinming-Hu]

@Jinming-Hu if you have it available, can you share links/info on proof of concepts/PRs by storage SDK owners for how they plan to implement this storage STG94 feature work in other languages? It would be helpful to see how the storage scenario is enabled using existing customization options for Java/.NET/Go/etc. for us to design how C++ should customize the RetryPolicy.

.Net PR Azure/azure-sdk-for-net#42845

The key logic is in sdk/storage/Azure.Storage.Common/src/Shared/StorageResponseClassifier.cs

[Jinming-Hu]

We need to document our design for retry policy modifications.

I was curious to see how other languages extend this capability, since this is a language agnostic feature.

One approach is making the RetryPolicy itself inheritable and expose customization capabilities on logic that can be injected before and after each retry attempt, and have the SDK author customize when to retry.

For example, .NET does this by having virtual/overridable methods including:

• ShouldRetry
• OnSendingRequest
• OnRequestSent

That said, I didn't see many compelling examples of service SDKs overriding the OnSendingRequest / OnRequestSent methods.

The other approach is to add the customization logic to the RetryOptions.

Javas has a ShouldRetryCondition predicate as part of their RetryOptions to customize the logic for when to actually retry a request. This is similar to Go, which has a ShouldRetry predicate func as part of its RetryOptions: https://github.com/Azure/azure-sdk-for-go/blob/01eb1703eea5f83182942e2865feedda0e162177/sdk/azcore/policy/policy.go#L126-L133

We could do something similar in C++ and add that field in the options.

There's actually another approach. As this is specific to storage service, any other service doesn't have this requirement, I can just copy the whole implementation of RetryPolicy and put it into storage package, this way you don't need to make any changes in core SDK. Please let me know if this works for you. @ahsonkhan

[ahsonkhan]

@Jinming-Hu here's the proposed solution for this customization: #5584

We don't want the custom logic of the base RetryPolicy to be copied in multiple places (since it introduces brittleness and a maintenance burden when we'd have to modify it).

Also, exposing the ShouldRetryOnResponce as a overridable extension point isn't desirable because it wasn't designed for that and does quite a few things, including managing the attempt count and jitter. We don't want the SDK author to have to implement that logic. Hence, the ShouldRetry API is simple and has a single responsibility, based on the Response and RetryOptions only.

Please take a look at my PR, provide any feedback you may have, and adjust your PR accordingly. I encourage you to keep aspects of this storage PR which includes cleaning up the pipeline and implementing the custom RetryPolicy for your scenario.

[antkmsft]

Now that #5584 is checked in, this PR is probably no longer needed/needs to be updated/can be closed?