Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Event Hubs] Introduce timeoutInMs #4239

Merged
merged 9 commits into from
Jul 11, 2019
Merged

[Event Hubs] Introduce timeoutInMs #4239

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1d1975e
Exclude receiver related changes
ramya0820 Jul 11, 2019
badb691
Address comments
ramya0820 Jul 11, 2019
77b2968
Update sdk/eventhub/event-hubs/src/eventHubClient.ts
ramya0820 Jul 11, 2019
2736e34
Merge branch 'master' into issue-4171-eh-only
ramya0820 Jul 11, 2019
2884ff4
Address comments
ramya0820 Jul 11, 2019
6ad3edb
Merge branch 'issue-4171-eh-only' of https://github.com/ramya0820/azu…
ramya0820 Jul 11, 2019
ad2cd9b
Rename to timeoutInMs
ramya0820 Jul 11, 2019
5fac7c6
Update docs per newer requirements
ramya0820 Jul 11, 2019
f7ffdea
Fix options type
ramya0820 Jul 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions sdk/eventhub/event-hubs/src/eventHubClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ export interface RetryOptions {
* Number of milliseconds to wait between attempts.
*/
retryInterval?: number;
/**
* Number of milliseconds to wait before declaring that current attempt has timed out which will trigger a retry
*/
operationTimeoutInMs?: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I would go with timeoutInMs for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

// /**
// * The maximum value the `retryInterval` gets incremented exponentially between retries.
// * Not applicable, when `isExponential` is set to `false`.
Expand All @@ -47,6 +51,17 @@ export interface RetryOptions {
// isExponential?: boolean;
}

export function getRetryOperationTimeoutInMs(retryOptions: RetryOptions | undefined): number {
const operationTimeoutInMs =
retryOptions == undefined ||
typeof retryOptions.operationTimeoutInMs !== "number" ||
!isFinite(retryOptions.operationTimeoutInMs) ||
retryOptions.operationTimeoutInMs < Constants.defaultOperationTimeoutInSeconds * 1000
? Constants.defaultOperationTimeoutInSeconds * 1000
: retryOptions.operationTimeoutInMs;
return operationTimeoutInMs;
}

/**
* The set of options to configure the behavior of an `EventHubProducer`.
* These can be specified when creating the producer via the `createProducer` method.
Expand Down
6 changes: 2 additions & 4 deletions sdk/eventhub/event-hubs/src/eventHubSender.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { ConnectionContext } from "./connectionContext";
import { LinkEntity } from "./linkEntity";
import { SendOptions, EventHubProducerOptions } from "./eventHubClient";
import { AbortSignalLike, AbortError } from "@azure/abort-controller";
import { getRetryOperationTimeoutInMs } from "./eventHubClient";

/**
* @ignore
Expand Down Expand Up @@ -610,10 +611,7 @@ export class EventHubSender extends LinkEntity {
this._sender!.on(SenderEvents.rejected, onRejected);
this._sender!.on(SenderEvents.modified, onModified);
this._sender!.on(SenderEvents.released, onReleased);
waitTimer = setTimeout(
actionAfterTimeout,
Constants.defaultOperationTimeoutInSeconds * 1000
);
waitTimer = setTimeout(actionAfterTimeout, getRetryOperationTimeoutInMs(options!.retryOptions));
const delivery = this._sender!.send(message, tag, 0x80013700);
log.sender(
"[%s] Sender '%s', sent message with delivery id: %d and tag: %s",
Expand Down
10 changes: 3 additions & 7 deletions sdk/eventhub/event-hubs/src/managementClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
import { ConnectionContext } from "./connectionContext";
import { LinkEntity } from "./linkEntity";
import * as log from "./log";
import { RetryOptions } from "./eventHubClient";
import { RetryOptions, getRetryOperationTimeoutInMs } from "./eventHubClient";
import { AbortSignalLike } from "@azure/abort-controller";
/**
* Describes the runtime information of an Event Hub.
Expand Down Expand Up @@ -303,12 +303,7 @@ export class ManagementClient extends LinkEntity {
*/
private async _makeManagementRequest(
request: Message,
options?: {
retryOptions?: RetryOptions;
timeout?: number;
abortSignal?: AbortSignalLike;
requestName?: string;
}
options?: { retryOptions?: RetryOptions; abortSignal?: AbortSignalLike; requestName?: string }
): Promise<any> {
try {
log.mgmt(
Expand All @@ -327,6 +322,7 @@ export class ManagementClient extends LinkEntity {
maxRetries: options.retryOptions && options.retryOptions.maxRetries,
abortSignal: options.abortSignal,
requestName: options.requestName,
timeoutInSeconds: getRetryOperationTimeoutInMs(options.retryOptions) / 1000,
delayInSeconds:
options.retryOptions &&
options.retryOptions.retryInterval &&
Expand Down