[Service Bus] Allow timeout lesser than 60 seconds #8805

HarshaNalluru · 2020-05-08T19:11:11Z

Removed the getRetryAttemptTimeoutInMs from service-bus, made necessary changes at the SBC level itself.
Updated event-hubs' getRetryAttemptTimeoutInMs to be in sync with the new proposal.
Invalid and non-positive values default to the defaultOperationTimeout

…rvice-bus to the core-amqp

richardpark-msft · 2020-05-08T20:24:57Z

sdk/core/core-amqp/src/retry.ts

@@ -26,6 +26,27 @@ function isDelivery(obj: any): boolean {
  return result;
 }

+/**
+ * TODO: We should question whether it's even a legitimate way of setting the timeout


A faithful port, even of the TODO comment questioning the legitimacy of the function. ;)

richardpark-msft · 2020-05-08T20:28:10Z

sdk/core/core-amqp/src/retry.ts

+ * @param {(RetryOptions | undefined)} retryOptions
+ * @returns {number}
+ */
+export function getRetryAttemptTimeoutInMs(retryOptions: RetryOptions | undefined): number {


Are we just going to remove this function anyways though when you go through and obliterate the "must be 60 seconds or less"? Ie, should we just not bother doing this so we don't have to re-release core-amqp?

richardpark-msft

Do we even need to bother with this? I think you have another issue that will obviate this entire function's existence.

ramya-rao-a · 2020-05-08T20:29:38Z

sdk/core/core-amqp/src/retry.ts

+    typeof retryOptions.timeoutInMs !== "number" ||
+    !isFinite(retryOptions.timeoutInMs) ||
+    // TODO: not sure what the justification is for always forcing at least 60 seconds.
+    retryOptions.timeoutInMs < Constants.defaultOperationTimeoutInMs


Now that we have decided to not put a min value of 60 seconds, the only thing we are doing is to replace invalid values with the default value. In most places before we get to using getRetryAttemptTimeoutInMs, we would have already defaulted to {} if there was no retry options. So, the invalid values being referred to are

not being a number

not being finite

not being 0 or less

Can we not make these checks in one single place i.e when user creates ServiceBusClient and remove the need for the helper altogether?

If we are going to have a helper method for this, then shouldnt other properties in the retry options also get that treatment?

Currently we dont, because the retry() method takes care of using the right defaults and timeout is the only retry option that is used outside of the retry() method.

What am getting to is that by looking at what is exposed from core-amqp, it is not clear why timeout is the special one that needs special handling.

Can we not make these checks in one single place i.e when user creates ServiceBusClient and remove the need for the helper altogether?

Just did that for service-bus.

What am getting to is that by looking at what is exposed from core-amqp, it is not clear why timeout is the special one that needs special handling.

Un-done changes to core-amqp

HarshaNalluru · 2020-05-12T21:00:58Z

sdk/servicebus/service-bus/src/util/utils.ts

- * @internal
- * @ignore
- */
-export function normalizeRetryOptions(


Since this was already done at SBC level, I see no point in keeping it now.

…to harshan/sb/issue/8192-part5

ramya-rao-a · 2020-05-13T00:22:18Z

Fixes #8193

richardpark-msft · 2020-05-13T00:24:27Z

sdk/servicebus/service-bus/src/serviceBusClient.ts

-      this._clientOptions.retryOptions
-    );
+
+    // Invalid timeouts, non-positive timeouts are defaulted to the `Constants.defaultOperationTimeoutInMs`


Why is it that we don't just throw an Error?

richardpark-msft · 2020-05-13T00:26:03Z

sdk/servicebus/service-bus/src/serviceBusClient.ts

+    // Invalid timeouts, non-positive timeouts are defaulted to the `Constants.defaultOperationTimeoutInMs`
+    const timeoutInMs = this._clientOptions.retryOptions.timeoutInMs;
+    this._clientOptions.retryOptions.timeoutInMs =
+      typeof timeoutInMs !== "number" || !isFinite(timeoutInMs) || timeoutInMs <= 0


Why would zero have to be mapped? It's valid?

richardpark-msft

This looks fine to me. I feel a little bit odd that we remap zero but I think it makes sense in that it's more like "0 is basically the uninitialized state, and should just use a default value".

It does seem like we should just throw in the face of other bogus input, however.

ramya-rao-a · 2020-05-15T18:38:35Z

Based on offline discussions, the decision is to throw errors when the timeout value is 0 or -ve value

ramya-rao-a · 2020-05-15T18:49:21Z

But now, would that be a breaking change? As someone passing -ve or 0 to event hubs would now get an error from the next update :(

HarshaNalluru · 2020-05-16T01:21:25Z

But now, would that be a breaking change?

Only update service-bus to throw errors for the invalid timeouts and leave event-hubs as it is, take care of event-hubs at the next major update?

ramya-rao-a · 2020-05-18T00:38:47Z

sdk/servicebus/service-bus/src/serviceBusClient.ts

+    if (typeof timeoutInMs !== "number" || !isFinite(timeoutInMs) || timeoutInMs <= 0) {
+      throw new Error(`${timeoutInMs} is an invalid value for retryOptions.timeoutInMs`);
+    }
+    this._clientOptions.retryOptions.timeoutInMs = timeoutInMs;


Do we have to set the timeout to the default value here? This forces us to use ! every time we want to use retryOptions.timeoutInMs in all other places.

Since the interface allows the timeout to not be set at all, I would suggest that we only do the error check here and copy over the timeoutInMs == undefined ? Constants.defaultOperationTimeoutInMs : timeoutInMs; to each place where the timeout is used.

Updated messageSender with this, the other methods already have this. And thus avoided ! at all the places.

So, now that all consumers of retry options will check for undefined and fallback to the default value, do you need the above where you set this._clientOptions.retryOptions.timeoutInMs?

Ah, if the timeout is undefined, typeof timeoutInMs !== "number" will be satisfied and an error is thrown. I thought it to be an overkill to allow undefined by placing more guards.

Then add the timeout != undefined to your if check

if (timeout != undefined && (typeof timeoutInMs !== "number" || !isFinite(timeoutInMs) || timeoutInMs <= 0))

ramya-rao-a · 2020-05-18T00:39:26Z

sdk/servicebus/service-bus/src/core/messageSender.ts

@@ -327,7 +327,7 @@ export class MessageSender extends LinkEntity {
        if (this._sender!.sendable()) {
          try {
            this._sender!.sendTimeoutInSeconds =
-              (this._retryOptions.timeoutInMs - timeTakenByInit) / 1000;
+              (this._retryOptions.timeoutInMs! - timeTakenByInit) / 1000;


In #8685, we added a guard to ensure this does not result in -ve value in Event Hubs. Can we include that in this PR for Service Bus

ramya-rao-a · 2020-05-18T00:40:04Z

Only update service-bus to throw errors for the invalid timeouts and leave event-hubs as it is, take care of event-hubs at the next major update?

That is fine by me, but in that case lets not make any changes to Event Hubs at all in this PR

sdk/servicebus/service-bus/src/serviceBusClient.ts

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

Move the duplicated getRetryAttemptTimeoutInMs from event-hubs and se…

21be5a1

…rvice-bus to the core-amqp

HarshaNalluru requested review from bterlson, chradek, ramya-rao-a and richardpark-msft as code owners May 8, 2020 19:11

changelog

9b84b74

HarshaNalluru mentioned this pull request May 8, 2020

[Core AMQP] Update getRetryAttemptTimeoutInMs to allow less than 60s timeout too #8806

Closed

3 tasks

richardpark-msft reviewed May 8, 2020

View reviewed changes

richardpark-msft requested changes May 8, 2020

View reviewed changes

ramya-rao-a reviewed May 8, 2020

View reviewed changes

HarshaNalluru added 5 commits May 12, 2020 13:08

updates as suggested

773b838

Not export getRetryAttemptTimeoutInMs from core-amqp

13f41dc

undo changelog

c9f425e

undo version updates

d9b7c9a

fix event-hubs

71285b0

HarshaNalluru changed the title ~~[Core AMQP] De-duplicating getRetryAttemptTimeoutInMs helper method~~ [Core AMQP] Fix getRetryAttemptTimeoutInMs helper method May 12, 2020

HarshaNalluru added 2 commits May 12, 2020 13:35

remove excess space

aecb21c

remove the helper method in service-bus

048e928

HarshaNalluru commented May 12, 2020

View reviewed changes

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

ac9aa26

…to harshan/sb/issue/8192-part5

HarshaNalluru changed the title ~~[Core AMQP] Fix getRetryAttemptTimeoutInMs helper method~~ [Service Bus] Remove getRetryAttemptTimeoutInMs helper method May 12, 2020

HarshaNalluru requested review from ramya-rao-a and richardpark-msft May 12, 2020 21:20

ramya-rao-a approved these changes May 13, 2020

View reviewed changes

ramya-rao-a changed the title ~~[Service Bus] Remove getRetryAttemptTimeoutInMs helper method~~ [Service Bus][Event Hubs] Allow timeout lesser than 60 seconds May 13, 2020

richardpark-msft reviewed May 13, 2020

View reviewed changes

richardpark-msft approved these changes May 13, 2020

View reviewed changes

throw errors for invalid timeouts

d57b3d3

HarshaNalluru requested a review from ramya-rao-a May 16, 2020 01:22

ramya-rao-a reviewed May 18, 2020

View reviewed changes

HarshaNalluru added 3 commits May 18, 2020 10:48

this._retryOptions.timeoutInMs! <= timeTakenByInit guard

1d96971

get rid of ! from timeoutInMs!

afc003b

Undo changes to event-hubs

3195fda

HarshaNalluru changed the title ~~[Service Bus][Event Hubs] Allow timeout lesser than 60 seconds~~ [Service Bus] Allow timeout lesser than 60 seconds May 18, 2020

HarshaNalluru requested a review from ramya-rao-a May 18, 2020 18:50

HarshaNalluru mentioned this pull request May 19, 2020

[Event Hubs v6] Allow retryTimeout lesser than 60 seconds #9017

Closed

Changes suggested by Ramya

d871475

ramya-rao-a reviewed May 20, 2020

View reviewed changes

sdk/servicebus/service-bus/src/serviceBusClient.ts Outdated Show resolved Hide resolved

Update sdk/servicebus/service-bus/src/serviceBusClient.ts

ccb6a4f

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

HarshaNalluru requested a review from ramya-rao-a May 20, 2020 22:24

ramya-rao-a approved these changes May 20, 2020

View reviewed changes

HarshaNalluru merged commit 1028761 into Azure:master May 20, 2020

ramya-rao-a mentioned this pull request May 21, 2020

[Service Bus] Timeout lesser than the default value is currently not allowed #8193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Service Bus] Allow timeout lesser than 60 seconds #8805

[Service Bus] Allow timeout lesser than 60 seconds #8805

HarshaNalluru commented May 8, 2020 •

edited

Loading

richardpark-msft May 8, 2020

richardpark-msft May 8, 2020

richardpark-msft left a comment

ramya-rao-a May 8, 2020

HarshaNalluru May 12, 2020 •

edited

Loading

HarshaNalluru May 12, 2020

ramya-rao-a commented May 13, 2020

richardpark-msft May 13, 2020

richardpark-msft May 13, 2020

richardpark-msft left a comment

ramya-rao-a commented May 15, 2020

ramya-rao-a commented May 15, 2020

HarshaNalluru commented May 16, 2020

ramya-rao-a May 18, 2020

HarshaNalluru May 18, 2020

ramya-rao-a May 19, 2020

HarshaNalluru May 19, 2020

ramya-rao-a May 20, 2020

HarshaNalluru May 20, 2020

ramya-rao-a May 18, 2020

HarshaNalluru May 18, 2020

ramya-rao-a commented May 18, 2020 •

edited

Loading

[Service Bus] Allow timeout lesser than 60 seconds #8805

[Service Bus] Allow timeout lesser than 60 seconds #8805

Conversation

HarshaNalluru commented May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

ramya-rao-a commented May 15, 2020

ramya-rao-a commented May 15, 2020

HarshaNalluru commented May 16, 2020

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 May 18, 2020 • edited Loading

HarshaNalluru commented May 8, 2020 •

edited

Loading

HarshaNalluru May 12, 2020 •

edited

Loading

ramya-rao-a commented May 18, 2020 •

edited

Loading