Add support for configuring OperationTimeout in the connection string

[0xced]

Like in Microsoft.ServiceBus.Messaging (.NET Framework), it's now possible to configure the OperationTimeout in the connection string.

The ServiceBusConnection class constructors are adapted to read the OperationTimeout from the connection string and the constructor explicitly having a TimeSpan operationTimeout argument is obsoleted with this message:

Please use the constructor with (string namespaceConnectionString, RetryPolicy retryPolicy) arguments and define the operationTimeout in the connection string.

Also, use the operation timeout defined in the connection string for the ManagementClient and actually use the operation timeout as a timeout for the HttpClient responsible for the management operations instead of storing the Constants.DefaultOperationTimeout in a private field which is never used.

[0xced]

Public API has to be updated for changes.

What does this mean? What do I have to do in addition to document the new ServiceBusConnection constructor and the new OperationTimeout property on the ServiceBusConnectionStringBuilder class? I don't see a CHANGELOG.md file, I don't understand what changes you are requesting. 😕

[SeanFeldman]

One of the tests, APIApprovals is failing. That's because the public API has changed and hasn't been approved. To do so, you'll need to run the test and update ApiApprovals.ApproveAzureServiceBus.approved.txt filed with the new content. This will also help to determine if there are any breaking changes or not. So far it looks like a new minor 🙂

[0xced]

It wasn't very clear from the failure since the error was about line ending mismatch:

Assert.Equal() Failure
                                 ↓ (pos 443)
Expected: ···edf0bdbccafadfeb6")]\n[assembly: System.Runtime.InteropService···
Actual:   ···edf0bdbccafadfeb6")]\r\n[assembly: System.Runtime.InteropServic···
                                 ↑ (pos 443)

Anyway, I have updated the API approvals and the test is now passing.

[SeanFeldman]

Marked PR as [WIP] until it's not and ready to be merged.

[SeanFeldman]

BTW, I was recommending only the string representation use number of seconds, the SBConnectionStringBuilder.OperationTimeout should remain a TimeSpan for programmer ease.

@dlstucki yes, that's what everyone agreed upon. 👍

[SeanFeldman]

@0xced could you please

• Sign CLA to unblock merging
• Remove WIP from the title if you're no longer planning to modify anything
• Rebase to ensure it's all good

[0xced]

It's still a WIP, I'm waiting on an answer about the reasonable value range that should be accepted for the timeout. Also, I have signed the CLA, this is probably a glitch that will disappear the next time I push changes.

[nemakam]

And for reasonable value range , upper limit of 1 hour seems fine. But lets not have a lower limit.

[0xced]

lets not have a lower limit

How would the system behave if the timeout is set to zero seconds? Wouldn't it be a way to shoot yourself in the foot?

[nemakam]

Makes sense. I was initially thinking about letting them use 500ms operation timeout. But if we are moving to seconds format, then maybe we can have a minimum timeout of 1 second.

[SeanFeldman]

But lets not have a lower limit.

It's got to be greater than zero, no?

[SeanFeldman]

@0xced once you address https://github.com/Azure/azure-service-bus-dotnet/pull/644#discussion_r266104912, please remove [WIP] to indicate it's ready and I'll merge to kick off 3.4.0 release. Thanks 🙂

[SeanFeldman]

Thank you.