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 8 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
16 changes: 16 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,11 @@ 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
* A minimum value of 60 seconds will be used if a value not greater than this is provided.
*/
timeoutInMs?: number;
// /**
// * The maximum value the `retryInterval` gets incremented exponentially between retries.
// * Not applicable, when `isExponential` is set to `false`.
Expand All @@ -47,6 +52,17 @@ export interface RetryOptions {
// isExponential?: boolean;
}

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

/**
* 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 { getRetryAttemptTimeoutInMs } 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, getRetryAttemptTimeoutInMs(options!.retryOptions));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@chradek chradek Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updating the type had worked

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, getRetryAttemptTimeoutInMs } 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: getRetryAttemptTimeoutInMs(options.retryOptions) / 1000,
delayInSeconds:
options.retryOptions &&
options.retryOptions.retryInterval &&
Expand Down