Add Connected/Disconnected events #28379
Conversation
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
API change check for API changes have been detected in |
| @@ -69,7 +66,7 @@ internal class AmqpClient : TransportClient | |||
| /// | |||
| /// <param name="host">The fully qualified host name for the Service Bus namespace. This is likely to be similar to <c>{yournamespace}.servicebus.windows.net</c>.</param> | |||
| /// <param name="credential">The Azure managed identity credential to use for authorization. Access controls may be specified by the Service Bus namespace or the requested Service Bus entity, depending on Azure configuration.</param> | |||
| /// <param name="options">A set of options to apply when configuring the client.</param> | |||
| /// <param name="client">A set of options to apply when configuring the client.</param> | |||
There was a problem hiding this comment.
nit: Comment is no longer accurate.
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpConnectionScope.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// An event that can be subscribed to for notification when the client connects to the service. | ||
| /// </summary> | ||
| #pragma warning disable AZC0002 |
There was a problem hiding this comment.
nit: Comments for what these warnings are would be helpful.
| public virtual bool IsConnected => Connection.IsConnected; | ||
|
|
||
| /// <summary> | ||
| /// |
sdk/servicebus/Azure.Messaging.ServiceBus/src/Client/ServiceBusClient.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Client/ServiceBusClient.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// The proxy being used when communicating with the service. | ||
| /// </summary> | ||
| public IWebProxy Proxy { get; } |
There was a problem hiding this comment.
Proxy and TransportType are interesting additions. What's the scenario where they'd be useful? I'd see the client identifier being more helpful to differentiate between multiple instances.
There was a problem hiding this comment.
Yeah, I considered that but was thinking it might be better to wait until we actually expose Identifier on the client. Mostly, I just wanted to add the event args type now so that we can evolve it in the future.
| /// automatically on the next operation. As such, it is not recommended that this property is checked before performing operations with the <see cref="ServiceBusClient"/> | ||
| /// or any of the other Service Bus types. This property should be used only for diagnostic information. | ||
| /// </summary> | ||
| public virtual bool IsConnected => Connection.IsConnected; |
There was a problem hiding this comment.
We should also consider noting that this property does not indicate that read/publish operations are actively taking place.
| /// <remarks> | ||
| /// If the connection string is copied from the Service Bus entity itself, it will contain the name of the desired Service Bus entity, | ||
| /// and can be used directly without passing the name="entityName" />. The name of the Service Bus entity should be | ||
| /// passed only once, either as part of the connection string or separately. | ||
| /// </remarks> | ||
| internal ServiceBusConnection( | ||
| string connectionString, | ||
| ServiceBusClientOptions options) | ||
| ServiceBusClient client) |
There was a problem hiding this comment.
I have the same hesitation here that I mentioned in the transport client; this changes the flow of dependencies from a single direction into circular. Have we considered surfacing the connected/disconnected events from the transport client through the connection?
| @@ -473,11 +473,6 @@ public void MetricsPropertyDoesNotThrowWhenEnabled() | |||
| /// | |||
| public class ReadableOptionsMock : ServiceBusClient | |||
| { | |||
There was a problem hiding this comment.
We should consider some unit tests for the handlers here, since we can fully control when they fire and what args they emit.
| { | ||
| WasCloseCalled = true; | ||
| return Task.CompletedTask; | ||
| await DisconnectedAsync(new ServiceBusConnectionEventArgs(ServiceEndpoint.Host, ServiceBusTransportType.AmqpTcp, null)); |
There was a problem hiding this comment.
Need to update to add explicit tests for these.
Alternative approach to address #8561