5945 retrypolicy response headers

[hemanttanwar]

Currently, RetryPolicy uses retry-after-ms to determine the delay duration. However, different Azure services return the delay duration in different response headers.
So, we should look into supporting different headers for delay duration.
For e.g. AppConfig uses retry-after-ms while CosmosDB uses x-ms-retry-after-ms.
RetryPolicy should also allow a customized list of HTTP status codes that are retryable.
#5945

Fixes #5945

[srnagar]

This can be null. If the header is not provided, then the retry strategy will be used to determine the delay. If provided, the delay will be determined based on the header value. Also, you will need the TimeUnit as well. Can't assume the unit is always milliseconds. For e.g. the default HTTP header Retry-After returns delay in seconds.

[hemanttanwar]

retryAfterHeader : This could be null when user do not want to use any response header for retry.

[hemanttanwar]

retryAfterHeader : This could be null when user do not want to use any response header for retry. This is change of behavior from GA release, In GA we check for value of header 'retry-after-ms' by default. This is discussed with Srikanta

[anuchandy]

just brain storming: what do you guys think about offloading the entire retry delay computation logic to strategies? Instead of adding more Ctrs overloads to RetryPolicy.

Something like:

public interface RetryStrategy {

    default Duration Duration calculateRetryDelay(HttpResponse httpResponse, int retryAttempts) {
        // Look up RFC defined and Azure well known heades such as:
        // retry-after-ms, x-ms-retry-after-ms in a specific order that is agreed across SDKS
        //
        // case1: there is retry header
        Duration delay = httpResponse.headers.get('retry-after-ms') | httpResponse.headers.get('x-ms-retry-after-ms')
        return delay;
        // case 2: there is no retry header
        return calculateRetryDelay(retryAttempts);
    }

    Duration calculateRetryDelay(int retryAttempts);
}

This way, if someone want customization, they could override calculateRetryDelay(HttpResponse httpResponse, int retryAttempts) and look at any header, multiple header or any other fields of the request/response to decide the delay. This also address the case that @alzimmermsft of pointed - cases where service uses different headers to communicate the delay.

[itye-msft]

Hi guys,
Based on @JonathanGiles request, I am adding my recommendation to also modify the RequestIdPolicy.

Currently RequestIdPolicy adds x-ms-client-request-id header which is a legacy header.
The service seems to ignore this header, because it expects the new format header client-request-id.

The proposed change appends the new header in addition to the legacy one in order to maintain a backwards compatibility.

This way tracing will start working.

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.core.http.policy;

import com.azure.core.http.HttpPipelineCallContext;
import com.azure.core.http.HttpPipelineNextPolicy;
import com.azure.core.http.HttpResponse;
import reactor.core.publisher.Mono;

import java.util.UUID;

/**
 * The pipeline policy that puts a UUID in the request header. Azure uses the request id as
 * the unique identifier for the request.
 */
public class RequestIdPolicy implements HttpPipelinePolicy {
    private static final String REQUEST_ID_HEADER = "client-request-id";
    private static final String LEGACY_REQUEST_ID_HEADER = "x-ms-client-request-id";

    @Override
    public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
        String requestId = context.getHttpRequest().getHeaders().getValue(REQUEST_ID_HEADER);
        if (requestId == null) {
            String randomUUID = UUID.randomUUID().toString();
            context.getHttpRequest().getHeaders().put(REQUEST_ID_HEADER, randomUUID);

            //also set the legacy header for backwards compatibility.
            context.getHttpRequest().getHeaders().put(LEGACY_REQUEST_ID_HEADER, randomUUID);
        }
        return next.process();
    }
}

Edit:
Based on the docs (https://docs.microsoft.com/en-us/rest/api/searchservice/common-http-request-and-response-headers-used-in-azure-search) and Swagger spec, Search uses client-request-id and return-client-request-id.

[srnagar]

This overload should throw NPE if retryStrategy is null.

[hemanttanwar]

We tried to avoid many variations on constructor. Strategy cannot be null, Thus defaulting to ExponentialBackoff . Following constructor was removed.

public RetryPolicyOptions( String retryAfterHeader, ChronoUnit retryAfterTimeUnit) ;
trying to manage between many variations in constructor or few constructor.

[JonathanGiles]

I realise this has already been merged, but I'm not convinced of the value of the RetryPolicyOptions type. It seems to present more API complexity for not much more gain - unless I'm missing something?

[JonathanGiles]

@hemanttanwar I'm keen to discuss the comment I raised yesterday. Is there an argument for the RetryPolicyOptions type?

[anuchandy]

bringing up: #6196 (comment)

[anuchandy]

My proposal earlier was: Offload the entire retry delay computation logic to strategies Instead of adding more Ctrs overloads to RetryPolicy.

Something like:

public interface RetryStrategy {

    default Duration Duration calculateRetryDelay(HttpResponse httpResponse, int retryAttempts) {
        // Look up RFC defined and Azure well known heades such as:
        // retry-after-ms, x-ms-retry-after-ms in a specific order that is agreed across SDKS
        //
        // case1: there is retry header
        Duration delay = httpResponse.headers.get('retry-after-ms') | httpResponse.headers.get('x-ms-retry-after-ms')
        return delay;
        // case 2: there is no retry header
        return calculateRetryDelay(retryAttempts);
    }

    Duration calculateRetryDelay(int retryAttempts);
}

This way, if someone want customization, they could override calculateRetryDelay(HttpResponse httpResponse, int retryAttempts) and look at any header, multiple header or any other fields of the request/response to decide the delay. This also address the case that @alzimmermsft of pointed - cases where service uses different headers to communicate the delay.

\cc @srnagar

[anuchandy]

basically any method in RetryStrategy that takes a dynamic decision will have access to HttpResponse (and indirectly has access to HttpRequest as well). So AzConfig customization in AzConfigClientBuilder will looks like:

RetryPolicy DEFAULT_RETRY_POLICY = new RetryPolicy(new ExponentialBackoff() {
    @Override
    public  Duration calculateRetryDelay(HttpResponse httpResponse, int retryAttempts)) {
        String delay = httpResponse.getHeaderValue("retry-after-ms");
        if (delay != null) {
            return Duration.ofMillis(Long.parseLong(delay));
        } else {
            return calculateRetryDelay(retryAttempts);
        }
    }
});

policies.add(retryPolicy == null ? DEFAULT_RETRY_POLICY : retryPolicy);
policies.addAll(this.policies);
HttpPolicyProviders.addAfterRetryPolicies(policies);

[conniey]

I like @anuchandy 's proposed suggestion. Maybe opening up an issue so this doesn't get lost in this merged PR.

[anuchandy]

sure, created an issue #6363 to triage.

[hemanttanwar]

I have closed many related issue #6363 #6171 #6247 and moving all the discussion into one #6217

I have another PR #6400 to implement suggested changes by @anuchandy