[Event Hubs] Introduce timeoutInMs #4239

ramya0820 · 2019-07-10T01:29:53Z

Following is summary of usage of retryOptions and instances of applicable operation timeouts

makeManagementRequest relies on retryOptions to support send requests. This already has per try operation timeout implemented in core-amqp and just required us to make option available to users and pass it in
receive and receiveBatch operations already support per try operation timeouts and allow users to configure this value using maxWaitTimeInSeconds. However introducing operationTimeoutInMs on retryOptions to mean same thing is bound to get confusing. Should we get rid of this parameter and have users supply this using the retryOptions ? @ramya-rao-a

For more context refer to #4171

ramya0820 · 2019-07-10T01:31:56Z

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

-   * should wait to receiver the said amount of messages. If not provided, it defaults to 60 seconds.
+   * @param [maxWaitTimeInSeconds] 
+   * 
+   * This will no longer be used.


@ramya-rao-a Specifically anted to clarify if making this change to API is fine. This will subsequently require updates across source, tests and likely samples.

Lets keep receiver changes out of scope of this PR and re-visit later

ramya-rao-a · 2019-07-10T01:58:03Z

receive does not take any timeouts, only receiveBatch does. Keep receive operations out of scope of this PR. We will clarify this with the service team and get back to it

ramya-rao-a · 2019-07-10T01:59:13Z

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

+  /**
+   * Number of milliseconds to wait for each operation attempt to get completed within.
+   */
+  operationTimeoutInMs?: number;


Did we run the name by @bterlson?
I worry that operationTimeout would make one think that the time applies to the entire operation and not the individual attempts

Yes, the name 'operationTimeoutInMs' was decided on by Brian and it has always referred to the individual attempt/execution.

Currently though, this PR is scoped per specific changes you wanted in. The documentation/usage of RetryPolicy will need to be made clear and is likely going to have more changes based on further discussions/work we discussed offline for #2835

@bterlson Given the current context of RetryPolicy related changes/usage in this PR, could you clarify if this needs to change?

Since we're enforcing a minimum timeout value, I think we should document it here as well.

But that's only for exponential retry correct? As I see it, that isn't supported yet

To be fair, I only decided on the suffix! I agree with @ramya-rao-a that operationTimeout is misleading. perAttemptTimeoutInMs? Just timeoutInMs may be appropriate too...

@chradek was referring to #4239 (comment) when he spoke of the "minimum"

I would go with timeoutInMs for now.

ramya-rao-a · 2019-07-10T02:00:38Z

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

+    eventHubPath: string,
+    credential: TokenCredential,
+    options?: EventHubClientOptions
+  );


Can you check if you are using the prettier extension or not? These formatting changes shouldnt be happening

ramya-rao-a · 2019-07-10T02:04:56Z

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

+          options.retryOptions.operationTimeoutInMs == undefined ||
+          options.retryOptions.operationTimeoutInMs < 0
+            ? Constants.defaultOperationTimeoutInSeconds
+            : options.retryOptions.operationTimeoutInMs / 1000,


We had feedback that we should have a minimum value for per operation timeout, else it is easy to abuse the system by forcing frequent timeouts by giving smaller values.

Use the Constants.defaultOperationTimeoutInSeconds as the "minimum" for now

I see that 4ba2e14 commit has been made in an attempt to address the issue, but the change is not solving the problem at hand.

Use Constants.defaultOperationTimeoutInSeconds as minimum value means that if user provides a value lesser than this, then we ignore user's value and use Constants.defaultOperationTimeoutInSeconds instead

ramya-rao-a · 2019-07-10T02:05:19Z

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

-   * should wait to receiver the said amount of messages. If not provided, it defaults to 60 seconds.
+   * @param [maxWaitTimeInSeconds] 
+   * 
+   * This will no longer be used.


Lets keep receiver changes out of scope of this PR and re-visit later

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

ramya-rao-a · 2019-07-11T00:58:27Z

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

+              options.retryOptions.operationTimeoutInMs == undefined ||
+              options.retryOptions.operationTimeoutInMs <= 0
+              ? Constants.defaultOperationTimeoutInSeconds * 1000
+              : options.retryOptions.operationTimeoutInMs


For readability, pull this code out and have a variable/constant hold the value of the timeout

Initializing options with {} is done early on, so the check options == undefined is not needed

Within the event handler it seemed to require the check, please see updated code

ramya-rao-a · 2019-07-11T00:58:53Z

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

@@ -296,6 +296,12 @@ export class ManagementClient extends LinkEntity {
        maxRetries: options.retryOptions && options.retryOptions.maxRetries,
        abortSignal: options.abortSignal,
        requestName: options.requestName,
+        timeoutInSeconds:
+          options.retryOptions == undefined ||


For readability, pull this code out and have a variable/constant hold the value of the timeout

ramya-rao-a · 2019-07-11T02:36:25Z

@ramya0820 Please avoid force pushing to an open PR, we lose track of all older comments in the Files view

Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>

chradek · 2019-07-11T17:57:29Z

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

+  /**
+   * Number of milliseconds to wait for each operation attempt to get completed within.
+   */
+  operationTimeoutInMs?: number;


Since we're enforcing a minimum timeout value, I think we should document it here as well.

chradek · 2019-07-11T18:01:31Z

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

@@ -471,6 +471,13 @@ export class EventHubSender extends LinkEntity {
      options = {};
    }

+    const operationTimeoutInMs =
+      options.retryOptions == undefined ||
+        options.retryOptions.operationTimeoutInMs == undefined ||


Maybe a safer check would be

Suggested change

options.retryOptions.operationTimeoutInMs == undefined ||

typeof options.retryOptions.operationTimeoutInMs !== "number" ||

That way we don't allow it to be set to a different type in the last part of your ternary operation. Could also add an isFinite(options.retryOptions.operationTimeoutInMs so we protect against NaN or Infinity (unless we want Infinity as a valid value).

Nice, we should probably add these validations to other such parameters as well

chradek · 2019-07-11T18:03:59Z

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

@@ -323,10 +318,18 @@ export class ManagementClient extends LinkEntity {
        options = {};
      }

+      const operationTimeoutInMs =
+        options.retryOptions == undefined ||


This logic is duplicated in eventHubReceiver, and will probably be used by the receiver at some point. Could you refactor this into its own function that takes in either retryOptions or operationTimeoutInMs, and then reference this function in both places?

Finding a place to house a utility like that seemed a bit tricky. Updated/created one along side RetryOptions interface definition in eventHubClient file.

Moreover, this may change and all required validations would likely be done in one place as part of retry module within core-amqp eventually.

…re-sdk-for-js into issue-4171-eh-only

chradek · 2019-07-11T21:55:47Z

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

-            actionAfterTimeout,
-            Constants.defaultOperationTimeoutInSeconds * 1000
-          );
+          waitTimer = setTimeout(actionAfterTimeout, getRetryAttemptTimeoutInMs(options!.retryOptions));


Just curious, was the TypeScript compiler unable to tell that options is always defined?

Yeah, in the IDE I was seeing a squiggly prompt that it may be undefined..

@chradek Would you prefer options && options.retryOptions over options!.retryOptions.
I am aware of the knee jerk reaction we get when we see the ! :)
Was just curious about what you felt about the above alternative

I actually wonder if changing

private _trySendBatch( message: AmqpMessage | Buffer, tag: any, options?: SendOptions & EventHubProducerOptions, format?: number ): Promise<void> { if (!options) { options = {}; }

to

private _trySendBatch( message: AmqpMessage | Buffer, tag: any, options: SendOptions & EventHubProducerOptions = {}, format?: number ): Promise<void> {

would solve the squiggles instead since then the compiler should see options always exists.

Edit: Though if that doesn't work, I'd choose the options && options.retryOptions route instead.

Updating the type had worked

* [Event Hubs] Introduce timeoutInMs on RetryOptions (#4239) * Update event-hubs.api.md file

* [Event Hubs] Introduce timeoutInMs on RetryOptions (#4239) * Update version in constants file

* [Event Hubs] Introduce timeoutInMs on RetryOptions (#4239) * Remove ENDPOINT env var * Remove ENDPOINT var * Update AAD env vars * Revert "Update AAD env vars" This reverts commit f6834cd. * Remove ref from tests.yml

ramya0820 requested a review from ramya-rao-a July 10, 2019 01:29

ramya0820 self-assigned this Jul 10, 2019

ramya0820 commented Jul 10, 2019

View reviewed changes

ramya-rao-a suggested changes Jul 10, 2019

View reviewed changes

Exclude receiver related changes

1d1975e

ramya0820 force-pushed the issue-4171-eh-only branch from c5257d8 to 1d1975e Compare July 11, 2019 00:12

ramya-rao-a reviewed Jul 11, 2019

View reviewed changes

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

ramya-rao-a reviewed Jul 11, 2019

View reviewed changes

Address comments

badb691

ramya0820 force-pushed the issue-4171-eh-only branch from 4ba2e14 to badb691 Compare July 11, 2019 02:15

ramya-rao-a requested review from ShivangiReja and chradek July 11, 2019 02:39

ramya0820 and others added 2 commits July 11, 2019 10:33

Update sdk/eventhub/event-hubs/src/eventHubClient.ts

77b2968

Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>

Merge branch 'master' into issue-4171-eh-only

2736e34

chradek reviewed Jul 11, 2019

View reviewed changes

ramya0820 added 4 commits July 11, 2019 11:53

Address comments

2884ff4

Merge branch 'issue-4171-eh-only' of https://github.com/ramya0820/azu…

6ad3edb

…re-sdk-for-js into issue-4171-eh-only

Rename to timeoutInMs

ad2cd9b

Update docs per newer requirements

5fac7c6

chradek approved these changes Jul 11, 2019

View reviewed changes

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

View reviewed changes

Fix options type

f7ffdea

ramya0820 merged commit 40fe1af into Azure:master Jul 11, 2019

ramya0820 changed the title ~~[Event Hubs] Introduce operationTimeoutInMs~~ [Event Hubs] Introduce timeoutInMs Jul 11, 2019

ramya0820 added a commit to ramya0820/azure-sdk-for-js that referenced this pull request Jul 11, 2019

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

260b0e6

ramya0820 mentioned this pull request Jul 11, 2019

[Event Hubs] Update event-hubs.api.md file #4280

Merged

ramya0820 added a commit that referenced this pull request Jul 11, 2019

[Event Hubs] Update event-hubs.api.md file (#4280)

4125def

* [Event Hubs] Introduce timeoutInMs on RetryOptions (#4239) * Update event-hubs.api.md file

ramya-rao-a mentioned this pull request Jul 12, 2019

[Event Hubs] Retry options should support per try timeout #4171

Closed

ramya0820 added a commit that referenced this pull request Jul 16, 2019

[Service Bus] Update version in constants.ts (#4332)

fd7ac94

* [Event Hubs] Introduce timeoutInMs on RetryOptions (#4239) * Update version in constants file

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Event Hubs] Introduce timeoutInMs #4239

[Event Hubs] Introduce timeoutInMs #4239

ramya0820 commented Jul 10, 2019 •

edited

Loading

ramya0820 Jul 10, 2019

ramya-rao-a Jul 10, 2019

ramya-rao-a commented Jul 10, 2019

ramya-rao-a Jul 10, 2019

ramya0820 Jul 11, 2019

ramya0820 Jul 11, 2019

chradek Jul 11, 2019

ramya0820 Jul 11, 2019

bterlson Jul 11, 2019

ramya-rao-a Jul 11, 2019

ramya0820 Jul 11, 2019

ramya-rao-a Jul 10, 2019

ramya0820 Jul 11, 2019

ramya-rao-a Jul 10, 2019

ramya-rao-a Jul 11, 2019

ramya0820 Jul 11, 2019

ramya-rao-a Jul 10, 2019

ramya-rao-a Jul 11, 2019

ramya0820 Jul 11, 2019

ramya-rao-a Jul 11, 2019

ramya-rao-a commented Jul 11, 2019

chradek Jul 11, 2019

chradek Jul 11, 2019

ramya0820 Jul 11, 2019

chradek Jul 11, 2019

ramya0820 Jul 11, 2019

chradek Jul 11, 2019

ramya0820 Jul 11, 2019

ramya-rao-a Jul 11, 2019

chradek Jul 11, 2019 •

edited

Loading

ramya0820 Jul 11, 2019

	options.retryOptions.operationTimeoutInMs == undefined \|\|
	typeof options.retryOptions.operationTimeoutInMs !== "number" \|\|

[Event Hubs] Introduce timeoutInMs #4239

[Event Hubs] Introduce timeoutInMs #4239

Conversation

ramya0820 commented Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Jul 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya0820 commented Jul 10, 2019 •

edited

Loading

chradek Jul 11, 2019 •

edited

Loading