Customizing azure-core Http Header in RetryPolicy #6217 #6400

hemanttanwar · 2019-11-18T23:02:32Z

Removing RetryPolicyOptions used by RetryPolicy . It was not present in GA release as well.
Supporting Client to overwrite Custom 'Retry-After' Header in RetryPolicy .

Using Option 4 from discussion here -> #6217 (comment)
More discussion here #6217

Supporting ability to set multiple id header in RequestIdPolicy :
Search team need these headers
"x-ms-client-request-id
"client-request-id"

Azure-App Config need these headers
"x-ms-client-request-id"
"x-ms-correlation-request-id"

More detail discussion : #6217
Relevant PR from search team for reference: https://github.com/Azure/azure-sdk-for-java/pull/6214/files

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RetryPolicy.java

...pconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java

JonathanGiles · 2019-11-19T07:52:24Z

I'm not understanding this PR. The initial design requirement was to support custom headers. This was introduced in the previously-merged PR #6196 that added the RetryPolicyOptions type, which I have suggested isn't the right approach. This PR takes this out, which is good, but instead appears to focus on customising the retry delay (which I don't think has been a feature request yet), whilst making the requested ability to support custom headers much more complicated. Have you worked through the requirements you've outlined in #6217 and determined how they will be implemented? Why is this custom header requirement being conflated with the work on custom retry duration? Asking developers to implement the calculateRetryDelay method will lead to errors that we should try to avoid, primarily due to the logic in the default implementation that users may not recreate correctly.

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RetryPolicy.java

hemanttanwar · 2019-11-21T22:48:59Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+     * it is called. Example of these headers are 'x-ms-client-request-id', 'x-ms-correlation-request-id'.
+     */
+    public RequestIdPolicy(Supplier<HttpHeaders> requestIdSupplier) {
+        this.requestIdSupplier = requestIdSupplier;


We are using HttpHeaders because, There can be multiple headers which are of ID nature. We already have been asked for multiple id related headers by app config
"x-ms-client-request-id"
"x-ms-correlation-request-id"

Thus HttpHeaders represent multiple Headers which client can set.

sdk/core/azure-core/src/test/java/com/azure/core/http/policy/RequestIdPolicyTests.java

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

alzimmermsft · 2019-11-21T23:23:19Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+            if (Objects.nonNull(httpHeaders) && httpHeaders.getSize() > 0) {
+                httpHeaders.stream().forEach(httpHeader -> {
+                    String requestIdHeaderValue = context.getHttpRequest().getHeaders().getValue(httpHeader.getName());
+                    if (requestIdHeaderValue == null) {


This is a general question for all policies which mutate the request, should this just insert the header value?

@JonathanGiles @srnagar

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

alzimmermsft · 2019-11-21T23:27:27Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+
+        if (Objects.nonNull(requestIdSupplier)) {
+            HttpHeaders httpHeaders = requestIdSupplier.get();
+            if (Objects.nonNull(httpHeaders) && httpHeaders.getSize() > 0) {


Continuing from a previous comment, if the supplier is made non-nullable and we add a default header in the base constructor (and possible default to this is null is passed into the overloaded constructor), should we do nothing if the supplier returns nothing. x-ms-client-request-id is optional by many services, so this would be allowed, but it would hurt tracking issues.

You answered , I think, In order to track request, we do need request id. And if client does not provide or give us empty .. we should provide it.

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RetryPolicy.java

hemanttanwar · 2019-11-22T04:34:52Z

@itye-msft Please review RequestIDPolicy.

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

itye-msft

LGTM, thanks.
A specific test for RequestID with custom header would be nice if possible.

srnagar · 2019-11-22T18:56:56Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

+     * {@link HttpRequest}.
+     *
+     * @param requestIdSupplier to dynamically generate to request id for each {@link HttpRequest}. It is suggested
+     * that this {@link Supplier} provides unique value every time it is called.


What happens if the supplier provides a non-unique value? Will the service reject the request? How does the service use this request id?

When I spoke to Jeffrey Richter about this, I understood, request-id is for telemetry purpose. For example Cognitive Search request-id is optional. https://docs.microsoft.com/en-us/rest/api/searchservice/common-http-request-and-response-headers-used-in-azure-search
Optional caller-specified request ID, in the form of a GUID with no decoration such as curly braces (for example, client-request-id: 9C4D50EE-2D56-4CD3-8152-34347DC9F2B0). A caller-defined value that identifies the given request. If specified, this will be included in response information as a way to map the request.

srnagar · 2019-11-22T19:01:04Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java

    @Override
    public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) {
+
+        HttpHeaders httpHeaders = requestIdSupplier.get();


Wouldn't this allow users to provide a random set of HTTP headers and not just the request id headers?

For e.g. the supplier can return

new HttpHeaders().put("foo", "bar").put(REQUEST_ID_HEADER, UUID.randomUUID().toString());

yes . But unless we keep a bag of valid request id header, we can not validate. And whenever a new request-id header is introduced, we need to update the bag and release.

Implement RetryPolicy based on issue #6217

cb7cd8e

hemanttanwar mentioned this pull request Nov 18, 2019

[Azure-core ] Requirement Gathering- Customizing azure-core Http Policy #6217

Closed

Fixing import

ceba8b1

hemanttanwar requested review from JonathanGiles, alzimmermsft, conniey and srnagar and removed request for alzimmermsft November 18, 2019 23:32

anuchandy reviewed Nov 18, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RetryPolicy.java Show resolved Hide resolved

anuchandy reviewed Nov 18, 2019

View reviewed changes

...pconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java Outdated Show resolved Hide resolved

hemanttanwar mentioned this pull request Nov 18, 2019

5945 retrypolicy response headers #6196

Merged

Hemant Tanwar added 2 commits November 18, 2019 22:13

Using only one response header

1f0abed

Implement RetryPolicy based on issue #6217

14f37b0

hemanttanwar requested a review from anuchandy November 19, 2019 07:22

Hemant Tanwar added 2 commits November 19, 2019 11:58

Change design to read retry header while creating policy.

08b37ff

Change design to read retry header while creating policy.

cb1f4f4

hemanttanwar changed the title ~~Implement RetryPolicy based on issue #6217~~ Do Not Review Yet- Implement RetryPolicy based on issue #6217 Nov 19, 2019

Hemant Tanwar added 2 commits November 19, 2019 15:29

formatting

f5333dc

Code change based on review and option 4 design.

333e36e

hemanttanwar changed the title ~~Do Not Review Yet- Implement RetryPolicy based on issue #6217~~ Customizing azure-core Http Header in Http Policy #6217 Nov 20, 2019

Reving unused import statements.

20eadd4

hemanttanwar marked this pull request as ready for review November 20, 2019 23:06

hemanttanwar requested review from jianghaolu, mssfang and sima-zhu as code owners November 20, 2019 23:06

hemanttanwar commented Nov 20, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RetryPolicy.java Show resolved Hide resolved

hemanttanwar changed the title ~~Customizing azure-core Http Header in Http Policy #6217~~ Customizing azure-core Http Header in RetryPolicy #6217 Nov 21, 2019

Change comment

ad88092

Hemant Tanwar added 2 commits November 21, 2019 14:18

Adding support for multiple custom headers in RequestIDPolicy.

063dafd

Comment correction.

b4bbb15

hemanttanwar commented Nov 21, 2019

View reviewed changes

mssfang reviewed Nov 21, 2019

View reviewed changes

sdk/core/azure-core/src/test/java/com/azure/core/http/policy/RequestIdPolicyTests.java Show resolved Hide resolved

alzimmermsft reviewed Nov 21, 2019

View reviewed changes

incorporate review comment

23dee25

hemanttanwar requested a review from alzimmermsft November 22, 2019 04:32

mssfang reviewed Nov 22, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RequestIdPolicy.java Outdated Show resolved Hide resolved

itye-msft approved these changes Nov 22, 2019

View reviewed changes

hemanttanwar mentioned this pull request Nov 22, 2019

Do Not Review-rollback of retry policy options since it was not introduced at GA. #6514

Closed

alzimmermsft approved these changes Nov 22, 2019

View reviewed changes

anuchandy approved these changes Nov 22, 2019

View reviewed changes

Hemant Tanwar added 2 commits November 22, 2019 10:17

Added unit test case for client provided request id

e3f3ccd

Merge branch 'master' into 6217-retrypolicy-custom-responseheaders

878ccff

srnagar approved these changes Nov 22, 2019

View reviewed changes

Hemant Tanwar added 2 commits November 22, 2019 12:13

Merge branch 'master' into 6217-retrypolicy-custom-responseheaders

498a616

Fixing unit test

c076457

hemanttanwar requested a review from mssfang November 22, 2019 20:53

fix java doc

9c07867

hemanttanwar merged commit 3862907 into Azure:master Nov 23, 2019

hemanttanwar deleted the 6217-retrypolicy-custom-responseheaders branch November 23, 2019 00:26

hemanttanwar mentioned this pull request Nov 26, 2019

AzConfig: User provided Customized Headers Policy #6544

Merged

Customizing azure-core Http Header in RetryPolicy #6217 #6400

Customizing azure-core Http Header in RetryPolicy #6217 #6400

Uh oh!

Conversation

hemanttanwar commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonathanGiles commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hemanttanwar Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alzimmermsft Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alzimmermsft Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

hemanttanwar Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hemanttanwar commented Nov 22, 2019

Uh oh!

Uh oh!

itye-msft left a comment

Choose a reason for hiding this comment

Uh oh!

srnagar Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

hemanttanwar Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srnagar Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

hemanttanwar Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hemanttanwar commented Nov 18, 2019 •

edited

Loading

JonathanGiles commented Nov 19, 2019 •

edited

Loading

hemanttanwar Nov 22, 2019 •

edited

Loading

hemanttanwar Nov 22, 2019 •

edited

Loading