[Event Hubs] Update RetryOptions interface to pass exponential retry params by ramya0820 · Pull Request #4400 · Azure/azure-sdk-for-js

ramya0820 · 2019-07-23T18:52:47Z

For more context refer to #4266

…or-js into issue-4266

chradek · 2019-07-25T20:47:17Z

sdk/eventhub/event-hubs/src/eventHubReceiver.ts

        operation: () => this.initialize(initOptions),
        operationType: RetryOperationType.receiverLink,
-        maxRetries: Constants.defaultMaxRetriesForConnection
+        maxRetries: Constants.defaultMaxRetriesForConnection,


Super nit: looks like this change wasn't needed, does running npm run format remove the comma?

That's odd, it wasn't going away on file save. After merge from master it disappears on file save itself..

sdk/eventhub/event-hubs/src/receiver.ts

…or-js into issue-4266

ramya-rao-a · 2019-07-26T07:41:31Z

sdk/eventhub/event-hubs/src/receiver.ts

  private _initRetryOptions(
    retryOptions: RetryOptions = {}
-  ): Required<Pick<RetryOptions, "maxRetries" | "retryInterval">> {
+  ): Required<Pick<RetryOptions, "maxRetries" | "retryInterval">> & RetryOptions {


@chradek What was the intention of _initRetryOptions?
I initially thought it was a way to guarantee non undefined values on maxRetries and retryInterval

With this change, we now allow the rest of properties of RetryOptions here that may be undefined.
I have this feeling that the purpose is being defeated if initRetryOptions is not initializing with some value

@ramya-rao-a
The intention was indeed to make sure maxRetries and retryInterval were always defined since we have known default options for them. I think the method name is fine...a comment could be added to indicate that it will set default values if a default exists.

With that in mind, I think this should also set retryPolicy to required since we have a known default (Linear)

Sure, we can add the comment for clarity

About default for retryPolicy, core-amqp handles it at https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-amqp/src/retry.ts#L217

But agree that the value can be set and made required - not one way or against other since this is an internal utility. But we might want to rethink about why we need this utility and have any values be marked as required when they all by design seem to be optional.

This method was originally suggested so that we wouldn't have to keep calculating defaults every time a receiveBatch operation was called.

Since we are setting the defaults once when instantiating the class, we also know that these fields are always set. Making them appear Required lets us skip the null checks and avoid using the non-null assertion.

Got it, that makes sense in this context
Although I think the defaults for these retry related options can be managed, set within core-amqp's retry?
It doesn't seem like there's much value in doing it again, unless these were Event Hubs specific defaults~

Thanks, receiver.ts looks good

About classes like EventHubSender, since we are passing in the retryOptions as part of senderOptions current design/implementation doesn't seem to benefit from using a private instance. Because, we anyway we need to check contents of senderOptions from within send implementations that setup, perform retries.

If we do want to have one, then we'll need to refactor the internal API to not require passing senderOptions / retryOptions to eventHubSender and use the private instance set in receiver by moving implementation of send request and retries to sender as opposed to eventHubSender (Because for receive, we have it in receiver and not in eventHubReceiver.) We could refactor, improve the implementation as part of a different issue / PR.

Updated implementations for these however, to simplify and move out the checks where applicable.
Some instances of timeOutInMs still require default value computation as they are used in setTimeout. Few others such retryInterval need checks since they are in a differing units (seconds vs milliseconds) and can be updated as part of #4277

(Could consider naming it to be same as what we have for retry in core-amqp to likes of delayInMs?) @ramya-rao-a @bterlson

Discussed offline with @bterlson, we would like to update retryInterval to be delayInSeconds
@ramya-rao-a let us know if there's any concern with it.

@ramya0820 I would prefer not to do the renames of the retry options in this PR.
This PR should be scoped to

passing the exponential retry related options from user to relevant places

ensuring that this flow from user to the relevant places is using good coding practices

@bterlson will be consolidating the names across core-http and storage to come up with the final list and log an issue for it.

Few others such retryInterval need checks since they are in a differing units (seconds vs milliseconds) and can be updated as part of #4277

Agreed that we need to do the conversion and so need checks, but we dont need to set the defaults as we expect core-amqp to do that. So I would suggest something like the below

typeof retryInterval === "number" ? retryInterval/1000 : undefined

sdk/eventhub/event-hubs/src/receiver.ts

ramya-rao-a

3 observations:

In the getMaxMessageSize() is a function that uses retry. It should also use the exponential parameters as well
I like what we have done in the receiver.ts file i.e have all the defaults handled in core-amqp and avoid the retryOptions && retryOptions.maxRetries model in favor of retryOptions.amxRetries. Let's ensure we do the same in the eventHubSender.ts file
I believe #4322 is almost done. Let's get that merged, and then update this PR to pass in the exponential things to the management operations

ramya0820 · 2019-07-29T21:43:09Z

In the getMaxMessageSize() is a function that uses retry. It should also use the exponential parameters as well

Updated

I like what we have done in the receiver.ts file i.e have all the defaults handled in core-amqp and avoid the retryOptions && retryOptions.maxRetries model in favor of retryOptions.amxRetries. Let's ensure we do the same in the eventHubSender.ts file

Updated

I believe [Event Hubs] Perform sendRequest() operation handling from SDK #4322 is almost done. Let's get that merged, and then update this PR to pass in the exponential things to the management operations

Updated, yes depending on which PR gets merged first, I can take care of the conflicts and ensure this is in place. (The other one's waiting on another approval)

cc: @ramya-rao-a

ramya-rao-a · 2019-07-29T23:03:30Z

sdk/eventhub/event-hubs/src/receiver.ts

      connectionHost: this._context.config.host,
      connectionId: this._context.connectionId,
-      delayInSeconds: retryOptions.retryInterval,
+      delayInSeconds: retryOptions.delayInMs,


We are passing milliseconds to something that expects seconds

ramya-rao-a · 2019-07-29T23:06:57Z

sdk/eventhub/event-hubs/src/managementClient.ts

-            ? options.retryOptions.retryInterval / 1000
+          options.retryOptions.delayInMs &&
+          options.retryOptions.delayInMs >= 0
+            ? options.retryOptions.delayInMs / 1000


In the spirit of avoiding the null check for retryOptions we should initialize options.retryOptions to {} if no value was provided like we are doing in a couple of other places in this PR

Addressed this and other comments, please see latest commit

…to issue-4266

ramya-rao-a · 2019-07-30T00:35:02Z

sdk/eventhub/event-hubs/src/eventHubSender.ts

-        : Constants.defaultDelayBetweenOperationRetriesInSeconds;
+      typeof retryOptions.retryInterval === "number"
+        ? retryOptions.retryInterval / 1000
+        : undefined;


The above 2 constants are not needed. They can be inlined while creating the RetryConfig just like you did in getMaxMessageSize(). Then as part of #4401, the inline will get simplified too

ramya-rao-a · 2019-07-30T00:36:21Z

sdk/eventhub/event-hubs/src/managementClient.ts

-          : Constants.defaultDelayBetweenOperationRetriesInSeconds;
+        typeof retryOptions.retryInterval === "number"
+          ? retryOptions.retryInterval / 1000
+          : undefined;


The above 2 constants are not needed. They can be inlined while creating the RetryConfig just like you did in getMaxMessageSize(). Then as part of #4401, the inline will get simplified too

ramya-rao-a

Looks good. Just 2 comments regarding inlining maxRetries and delayInSeconds to be consistent

ramya0820 · 2019-07-30T18:44:47Z

Looks good. Just 2 comments regarding inlining maxRetries and delayInSeconds to be consistent

Updated

ramya0820 and others added 11 commits July 11, 2019 15:58

[Event Hubs] Introduce timeoutInMs on RetryOptions (Azure#4239)

260b0e6

Merge branch 'master' of https://github.com/ramya0820/azure-sdk-for-js

c74f5e4

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js

00016cb

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js

5c12c13

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js

2081256

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js

f3511e0

Update retryOptions

ee0994f

Update constructor

f5cf034

Update types

340c8bd

Minor errors

558f67e

Include api.md file

6131e97

ramya0820 requested review from chradek and ramya-rao-a July 23, 2019 18:52

ramya0820 self-assigned this Jul 23, 2019

ramya0820 and others added 3 commits July 23, 2019 11:55

Use timeOutInMs

8acb8ea

Merge branch 'issue-4266' of https://github.com/ramya0820/azure-sdk-f…

0263f0c

…or-js into issue-4266

Fix typo

c6e5d02

ramya-rao-a requested a review from ShivangiReja July 23, 2019 20:50

Merge branch 'master' into issue-4266

251b26c

chradek reviewed Jul 25, 2019

View reviewed changes

Ramya Raja added 2 commits July 25, 2019 14:21

Remove comma

d51b4c6

Merge branch 'issue-4266' of https://github.com/ramya0820/azure-sdk-f…

11b8c38

…or-js into issue-4266

ramya-rao-a reviewed Jul 26, 2019

View reviewed changes

chradek reviewed Jul 26, 2019

View reviewed changes

sdk/eventhub/event-hubs/src/receiver.ts Outdated Show resolved Hide resolved

sdk/eventhub/event-hubs/src/receiver.ts Outdated Show resolved Hide resolved

sdk/eventhub/event-hubs/src/receiver.ts Show resolved Hide resolved

Ramya Raja added 5 commits July 26, 2019 15:16

Address comments

a5a2f3d

Add comment

91c5bc3

Remove _initRetryOptions

ea840e3

Fix import

6a05bad

Simplify getOwnerLevel check

94a58e1

ramya-rao-a suggested changes Jul 29, 2019

View reviewed changes

Ramya Raja added 4 commits July 29, 2019 13:51

Simplify retryOptions handling in sender

4a25bd9

Update retryInterval -> delayInMs

c2bf51a

Add api.md file

09d5d42

Update management requests to include exponential retry related options

9a9c5f8

ramya-rao-a reviewed Jul 29, 2019

View reviewed changes

Ramya Raja added 4 commits July 29, 2019 16:46

Revert rename

2d6c112

Update retryInterval checks

aa495cd

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…

1b8c7cb

…to issue-4266

Add api.md file

d623472

ramya-rao-a reviewed Jul 30, 2019

View reviewed changes

ramya-rao-a suggested changes Jul 30, 2019

View reviewed changes

Uniformize usage

fdf729a

ramya-rao-a approved these changes Jul 30, 2019

View reviewed changes

ramya0820 merged commit 9011944 into Azure:master Jul 30, 2019

This was referenced Jul 30, 2019

[Event Hubs] Support exponential retry policy #4266

Closed

[core-amqp] Refactor time units to milliseconds #4401

Merged

Conversation

ramya0820 commented Jul 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a Jul 29, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ramya-rao-a left a comment

Choose a reason for hiding this comment

Uh oh!

ramya0820 commented Jul 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a Jul 30, 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!

ramya-rao-a Jul 30, 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!

ramya-rao-a left a comment

Choose a reason for hiding this comment

Uh oh!

ramya0820 commented Jul 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ramya-rao-a Jul 29, 2019 •

edited

Loading

ramya-rao-a Jul 30, 2019 •

edited

Loading

ramya-rao-a Jul 30, 2019 •

edited

Loading