Misc fixes by JoshLove-msft · Pull Request #10000 · Azure/azure-sdk-for-net

JoshLove-msft · 2020-02-15T06:08:29Z

Update client hierarchy based on our discussions
- 4 clients with 2 abstract base classes
- remove Receiver/Sender namespaces
Add RunOperation to RetryPolicy to help simplify retry logic
Remove SessionClient and put all session operations under new Session property on Receiver
Include SessionOptions in ctor overloads
Set the session filter when using a sessionful entity even if no session Id is populated

* Send session filter even if session Id is null if sessionOptions is passed * Remove SessionClient

JoshLove-msft · 2020-02-15T06:09:34Z

+    These analyzers are blocked by https://github.com/Azure/azure-sdk-tools/issues/127
+     -->
+    <NoWarn>
+      $(NoWarn);


This allows us to not match the ClientOption type names exactly with the Client type.
This is useful for SenderClientOptions. We can remove this if we end up creating explicit option types for each client.

JoshLove-msft · 2020-02-15T06:11:48Z

        /// </summary>
        ///
-        public ServiceBusRetryOptions Options { get; }
+        public override ServiceBusRetryOptions Options { get; }


Exposing this in ServiceBusRetryPolicy to provide access to the default timeout.

I'm not sure that you want to do this; the options type is mutable and intent of having this only on the internal implementation type was to ensure that we do not allow callers to change the options after the client was constructed.

If you're going to elevate, I'd suggest making the member internal.

jsquire · 2020-02-18T14:30:17Z

 using System.ComponentModel;
 using System.Diagnostics;
+using System.Threading;
+using System.Threading.Tasks;


Should this type be aware of the transport in use? For example, if we're doing JMS instead of AMQP or have a recorded test transport at some point, we would have to potentially extend this class since it has logic around AmqpException.

Would it make sense to, maybe, make this an extension method in the Amqp namespace, so that the AMQP transport types can access it, but other transport types could have a localized version with potentially different logic? (TranslateServiceException that is called was scoped to that AMQP namespace in Event Hubs; I don't believe that it moved moved during the Service Bus adoption)

Yeah it probably shouldn't know about AMQP. I think the only thing that wouldn't apply generically though is this exception type, so I'm not sure that it is worth moving this to an extension method. What do you think about intoducing the concept of a TransportException and using that here instead? AmqpException would derive from TransportException.

I'm not opposed to a transport exception, but I'm not sure it'll work the way that you're proposing. The AmqpException is part of the AMQP library and not one of ours, unfortunately. You'd have to wrap it. They do some specific things with how it wraps inner exceptions and such, which is why that translation method came into play, I'm not sure that we'd want that to be applied generically.... so maybe that translate could live in the AMQP area and if no other translations apply, wrap the AMQP exception in a transport exception and bubble it?

I'd like to figure this out as part of #9879

jsquire · 2020-02-18T14:38:26Z

+
+            try
+            {
+                TimeSpan tryTimeout = CalculateTryTimeout(0);


The per-try timeout doesn't appear to be considered within the operation. Should the signature for operation change to Func<TimeSpan, Task> so that it can be passed in and allow the operation itself to control whether it obeys the requested timeout period?

Just for context, most of the underlying AMQP library is based on timeouts rather than obeying cancellation. The closer you are to the transport, the more likely that you'll want/need to know the base timeout so you can pass it along.

Yeah, if we are going to support the per-try timeout, we would need to change to Func<TimeSpan, Task>

jsquire · 2020-02-18T14:41:57Z

+
+namespace Azure.Messaging.ServiceBus.Core
+{
+    internal abstract class TransportConnectionScope : IDisposable


nit: Missing doc comment for the class.

Will cover as part of doc updates

jsquire · 2020-02-18T14:52:22Z

        /// </summary>
        ///
-        public ServiceBusRetryOptions Options { get; }
+        public override ServiceBusRetryOptions Options { get; }


I'm not sure that you want to do this; the options type is mutable and intent of having this only on the internal implementation type was to ensure that we do not allow callers to change the options after the client was constructed.

If you're going to elevate, I'd suggest making the member internal.

jsquire · 2020-02-18T15:28:55Z

+            ReceivingAmqpLink openedLink = null;
+            await _retryPolicy.RunOperation(
+                async () =>
+                openedLink = await _consumer.ReceiveLink.GetOrCreateAsync(_retryPolicy.Options.TryTimeout).ConfigureAwait(false),


This confuses me. The point of the transport consumer is to abstract the concept of AMQP links away from callers, especially those outside of the AMQP layer. In its current form , this is leaking the abstraction. This makes it much harder to inject a transport consumer for behavioral overrides to unit test and to potentially add other transports (like a recording framework) in the future that may not be link-based.

I could be missing something, but shouldn't we be having the details of getting the session on the transport consumer or another transport client? I'd also advise against exposing the link directly, to avoid leaking that abstraction and force the consumer to be extended instead.

The implementation can be moved down to the consumer layer.

jsquire · 2020-02-18T15:32:41Z

+        /// <summary>
+        ///
+        /// </summary>
+        public ServiceBusConnection Connection { get; set; }


Maybe this was a detail from the design discussions that I missed and, if so, apologies but.... having the connection here feels orthogonal to the concerns of a session. The connection itself isn't session-aware or required to do something session-related?

Yeah our idea was that when using sessions, you need to provide a Connection. I think this may change though so this is still pretty tentative.

jsquire · 2020-02-18T15:33:24Z

+        ///
+        /// <remarks>
+        ///   If the connection string is copied from the Service Bus namespace, it will likely not contain the name of the desired Service Bus entity,
+        ///   which is needed.  In this case, the name can be added manually by adding ";EntityPath=[[ Service Bus entity NAME ]]" to the end of the


nit: The entity path capitalization is weird.

jsquire · 2020-02-18T15:36:51Z

@@ -27,7 +26,7 @@ namespace Azure.Messaging.ServiceBus
    ///
    /// <seealso href="https://docs.microsoft.com/en-us/Azure/event-hubs/event-hubs-about" />


nit: You may want to update the reference on line 28.

jsquire · 2020-02-18T15:38:09Z

        {
            Argument.AssertNotNull(retryPolicy, nameof(retryPolicy));
-            return InnerClient.CreateConsumer(retryPolicy, ownerLevel, prefetchCount, sessionId);
+            return InnerClient.CreateConsumer(retryPolicy, ownerLevel, prefetchCount, sessionId, isSessionReceiver);


At some point, we probably want to consider unifying terminology; if we're calling these "Sender/Receiver" rather than "Producer/Consumer" we'll probably want to update the member names.

JoshLove-msft added 2 commits February 14, 2020 20:35

* Fix retry logic

bc369d6

* Send session filter even if session Id is null if sessionOptions is passed * Remove SessionClient

Rebase

54396ad

JoshLove-msft commented Feb 15, 2020

View reviewed changes

Comment thread sdk/servicebus/Azure.Messaging.ServiceBus/src/Core/TransportConsumer.cs Outdated

JoshLove-msft commented Feb 15, 2020

View reviewed changes

Comment thread sdk/servicebus/Azure.Messaging.ServiceBus/src/ServiceBusRetryPolicy.cs Outdated

JoshLove-msft marked this pull request as ready for review February 17, 2020 20:08

JoshLove-msft requested review from jsquire and nemakam as code owners February 17, 2020 20:08

JoshLove-msft requested a review from ShivangiReja February 17, 2020 20:08

Add TransportConnectionScope

9eb0419

jsquire reviewed Feb 18, 2020

View reviewed changes

PR fixes

ef243d7

JoshLove-msft mentioned this pull request Feb 18, 2020

ServiceBusException issues #9879

Closed

nemakam removed their request for review February 18, 2020 20:51

jsquire approved these changes Feb 18, 2020

View reviewed changes

JoshLove-msft merged commit 8713e3f into Azure:master Feb 18, 2020

		@@ -27,7 +26,7 @@ namespace Azure.Messaging.ServiceBus
		///
		/// <seealso href="https://docs.microsoft.com/en-us/Azure/event-hubs/event-hubs-about" />

Conversation

JoshLove-msft commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshLove-msft Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JoshLove-msft commented Feb 15, 2020 •

edited

Loading

JoshLove-msft Feb 15, 2020 •

edited

Loading