[Service Bus] Time duration type change from ISO-8601 "string" to "number" #9430

HarshaNalluru · 2020-06-10T04:41:02Z

Issue - #9375

Attributes that follow ISO-8601 time duration format (string type)

autoDeleteOnIdle
defaultMessageTtl
duplicateDetectionHistoryTimeWindow
lockDuration

This PR attempts to change the type to "number" with the time duration in seconds.
With that, "InSeconds" suffix would be added at the end of those four attributes.

Please provide reviews and let's finalize on the attributes.

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

Co-authored-by: Richard Park <51494936+richardpark-msft@users.noreply.github.com>

HarshaNalluru · 2020-06-10T23:24:40Z

/azp run js - servicebus - tests

azure-pipelines · 2020-06-10T23:24:50Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/servicebus/service-bus/test/internal/utils.spec.ts

ramya-rao-a · 2020-06-15T17:05:20Z

sdk/servicebus/service-bus/review/service-bus.api.md

@@ -228,7 +228,7 @@ export interface PeekMessagesOptions extends OperationOptions {
 // @public
 export interface QueueDescription {
    authorizationRules?: AuthorizationRule[];
-    autoDeleteOnIdle?: string;
+    autoDeleteOnIdleInSeconds?: number;


Wasnt the guidance to use milli seconds in general in our JS SDKs? What data type are other languages using?

I did think of it, but usual values for this would be in "days", did not make much sense to convert days into milliseconds.

.NET has a special TimeSpan type for durations.

https://docs.microsoft.com/en-us/dotnet/api/system.timespan?view=netcore-3.1

Java is relying on a similar Duration type

https://github.com/Azure/azure-sdk-for-java/blob/3073499e73ca0d9d272942454f75819ecaf55330/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/models/QueueDescription.java#L51

Python has a similar datetime.timedelta

https://docs.python.org/2/library/datetime.html#datetime.timedelta

JS lacks such type for time duration.

It seems fine to me to use seconds for 2 reasons:

I think the ISO 8601 duration format only goes down to the second precision (though it does allow fractional seconds.)

arm-servicebus has these properties as well (without the InSeconds suffix) as strings. If the fields are named differently, then I think there will be less confusion about why one package returns a string and another returns a number.

Assuming that these properties need to work with larger timeframes like years to smaller values of seconds, are we sure that dealing with numbers is indeed more user friendly than using the ISO-8601 format?

One requires to do math, the other requires prior knowledge of ISO-8601 format.

Given that arm package will not be changing, is it worthwhile having separate treatment of these properties?

One requires to do math, the other requires prior knowledge of ISO-8601 format.

I had the same dilemma while porting the tests in this PR.

are we sure that dealing with numbers is indeed more user friendly than using the ISO-8601 format?

I would prefer ISO-8601 duration format over numbers. I don't know if it is conventional or convenient for the users.
If we prefer ISO-8601 duration format, we can add the ISO_8601#Durations link in the docs for all the attributes that take ISO-8601 string inputs.
This would also be consistent with the arm package.

Ah, I assumed that this had already been decided in an API review. If not then I'm also fine with keeping it as a string. When we get Temporal we can look at supporting that in the future.

The call to change this was quickly made in the .Net review, but I believe that was because they have a very convenient class Duration that is more user friendly than the ISO format.

cc @bterlson, @xirzec

After reflecting on this a bit more, it might be best to leave these as ISO strings until we get a better duration primitive to work with in JS.

sdk/servicebus/service-bus/review/service-bus.api.md

…to harshan/sb/issue/9375-part2

HarshaNalluru · 2020-06-16T18:48:12Z

/azp run js - servicebus - tests

azure-pipelines · 2020-06-16T18:48:23Z

Azure Pipelines successfully started running 1 pipeline(s).

HarshaNalluru · 2020-06-16T23:53:54Z

https://dev.azure.com/azure-sdk/internal/_build/results?buildId=427839&view=results

Green on Linux Node 8, Windows Node 10.
Failed on Mac Node 12 and on the browser with totally unrelated test failures.

HarshaNalluru · 2020-06-16T23:54:07Z

/azp run js - servicebus - tests

azure-pipelines · 2020-06-16T23:54:17Z

Azure Pipelines successfully started running 1 pipeline(s).

richardpark-msft

Looks good.

chradek · 2020-06-18T21:59:06Z

sdk/servicebus/service-bus/test/atomManagement.spec.ts

-      autoDeleteOnIdle: "P10675199DT2H48M5.4775807S",
-      defaultMessageTtl: "P10675199DT2H48M5.4775807S",
-      duplicateDetectionHistoryTimeWindow: "PT10M",
+      autoDeleteOnIdleInSeconds: 10675199 * 24 * 60 * 60 + 2 * 60 * 60 + 48 * 60 + 5.4775807, //"P10675199DT2H48M5.4775807S",


I'm having a hard time telling...do the atomManagement tests actually test that a response with the string duration is deserialized into the correct number?

These tests have historically been testing the response with the expected hard-coded output, my intention was just to migrate to the new type.

do the atomManagement tests actually test that a response with the string duration is deserialized into the correct number?

Not exactly, I have added separate unit tests for that - "string duration is deserialized into the correct number".

For this test, we do verify the response by matching it with this predefined output.
The response would contain the iso-8601 duration converted into seconds.
If the number from the response matches with the number here in the predefined output, the test would succeed and I would assume the string duration is deserialized into the correct number.

xirzec

Some minor comments and some confusion on my end

xirzec · 2020-06-18T22:15:57Z

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

+export function getISO8601DurationFromSeconds(
+  timeInSeconds: number | undefined
+): string | undefined {
+  if (timeInSeconds == null) {


can we make this more explicit?

Suggested change

if (timeInSeconds == null) {

if (timeInSeconds === undefined || timeInSeconds === null) {

This is interesting - why would we want to expand it? I was under the impression this was good practice?

https://basarat.gitbook.io/typescript/recap/null-undefined

Many developers are not familiar with the subtleties of non-strict comparisons, so they are often banned by lint rules. It's also easy to not notice a missing extra =.

And if you want to take it from Anders, we shouldn't ever use null in our code since it's not required by the language, but I'm not that much of a purist as to avoid it for use in sentinel values. ;)

Interesting.
timeInSeconds === undefined || timeInSeconds === null

To handle both null and undefined, we have been doing timeInSeconds == undefined check at other places.

xirzec · 2020-06-18T22:16:47Z

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

+  if (timeInSeconds == null) {
+    return undefined;
+  }
+  if (typeof timeInSeconds != "number") {


xirzec · 2020-06-18T22:19:11Z

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

+  if (timeInSeconds == null) {
+    return undefined;
+  }
+  if (typeof timeInSeconds != "number") {


what about if timeInSeconds is NaN? Or some other special value like Infinity?

xirzec · 2020-06-18T22:22:22Z

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

+  let timeSeparatorAdded = false;
+  for (const { label, inSeconds } of [day, hour, minute, second]) {
+    const value =
+      label == second.label


nit: another non-strict comparison

xirzec · 2020-06-18T22:28:11Z

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

+  const minute = { label: "M", inSeconds: 60 };
+  const hour = { label: "H", inSeconds: minute.inSeconds * 60 };
+  const day = { label: "D", inSeconds: hour.inSeconds * 24 };
+  const countDecimals =


I'm not really sure why we get how many decimal values there are, just to call toFixed of that value later. Are we trying to truncate it in a way that avoids floating point issues?

What's the case where this isn't true?

timeInSeconds.toString() === timeInSeconds.toFixed(timeInSeconds.toString().split(".")[1].length)

xirzec · 2020-06-18T22:35:36Z

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

+ * Example: PT10M, P106DT2H48M5.47S
+ *
+ * This helper method is to convert the duration in seconds to ISO-8601 duration format.
+ *


Can we mention here that we don't support weeks/months/years because the service doesn't send that?

Good point. I've added it in the comments for one of the methods, I'll add it in the docs too.

xirzec · 2020-06-18T22:35:55Z

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

+ * Example: PT10M, P106DT2H48M5.47S
+ *
+ * This helper method is to convert the duration in ISO-8601 format into seconds.
+ *


again a reference to not supporting years/months/weeks would be good

HarshaNalluru · 2020-06-22T05:51:00Z

Waiting for #9430 (comment) discussion to be addressed to decide the fate of the PR.. either addressing the rest of the feedback and merge or discarding the PR.

HarshaNalluru · 2020-06-25T21:37:30Z

Closing in favour of #9723

HarshaNalluru added 4 commits June 9, 2020 21:35

helper methods to convert to and from ISO-8601 format

a489fef

autoDeleteOnIdle -> autoDeleteOnIdleInSeconds

8edfba8

update tests appropriately

7f2d31b

API report

a88c233

richardpark-msft requested changes Jun 10, 2020

View reviewed changes

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

sdk/servicebus/service-bus/src/util/utils.ts Show resolved Hide resolved

HarshaNalluru and others added 7 commits June 10, 2020 13:59

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

9328dd3

Co-authored-by: Richard Park <51494936+richardpark-msft@users.noreply.github.com>

improve parse for ISO8601 Duration to seconds

fa122b6

Add comments for regex and minor fix for getISO8601DurationFromSeconds

85d03e1

Add base case 0s

cdd6cdb

final fixes to getISO8601DurationFromSeconds

e07adc6

getISO8601DurationFromSeconds - fix decimals

7e117d2

Add tests

dd060c5

HarshaNalluru marked this pull request as ready for review June 10, 2020 23:36

HarshaNalluru requested review from bterlson, chradek, qiaozha and ramya-rao-a as code owners June 10, 2020 23:36

HarshaNalluru requested a review from richardpark-msft June 10, 2020 23:36

richardpark-msft reviewed Jun 10, 2020

View reviewed changes

sdk/servicebus/service-bus/test/internal/utils.spec.ts Show resolved Hide resolved

ramya-rao-a reviewed Jun 15, 2020

View reviewed changes

HarshaNalluru added 8 commits June 15, 2020 17:31

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

90deee5

…to harshan/sb/issue/9375-part2

throw error for invalid inputs

20d7dee

test cases for invalid inputs

9031458

duplicated line

5421e53

Update rest of the attributes from QDesc

9060054

Update TopicDesc

fe1281b

Update SubDesc

6171712

update tests - first pass

bb55e32

HarshaNalluru added 4 commits June 15, 2020 19:02

fix test utils

313cb5c

fix a couple of tests

c5304db

API report

a471d88

add more tests

4b871bf

HarshaNalluru requested a review from richardpark-msft June 16, 2020 23:54

richardpark-msft approved these changes Jun 18, 2020

View reviewed changes

chradek reviewed Jun 18, 2020

View reviewed changes

xirzec reviewed Jun 18, 2020

View reviewed changes

ramya-rao-a added the Service Bus label Jun 22, 2020

ramya-rao-a removed the request for review from qiaozha June 22, 2020 06:59

This was referenced Jun 23, 2020

[Service Bus] ATOM Management followups #9375

Closed

[Service Bus] More info in the doc strings for ISO-8601 duration format #9723

Merged

HarshaNalluru closed this Jun 25, 2020

	if (timeInSeconds == null) {
	if (timeInSeconds === undefined \|\| timeInSeconds === null) {

[Service Bus] Time duration type change from ISO-8601 "string" to "number" #9430

[Service Bus] Time duration type change from ISO-8601 "string" to "number" #9430

Conversation

HarshaNalluru commented Jun 10, 2020 • edited Loading

Attributes that follow ISO-8601 time duration format (string type)

HarshaNalluru commented Jun 10, 2020

azure-pipelines bot commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Jun 15, 2020 • edited Loading

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

HarshaNalluru commented Jun 16, 2020

azure-pipelines bot commented Jun 16, 2020

HarshaNalluru commented Jun 16, 2020

HarshaNalluru commented Jun 16, 2020

azure-pipelines bot commented Jun 16, 2020

richardpark-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

xirzec left a 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

HarshaNalluru commented Jun 22, 2020

HarshaNalluru commented Jun 25, 2020

HarshaNalluru commented Jun 10, 2020 •

edited

Loading

HarshaNalluru Jun 15, 2020 •

edited

Loading

HarshaNalluru Jun 18, 2020 •

edited

Loading