RabbitMQ 4.0 support #1512
RabbitMQ 4.0 support #1512
Conversation
Pass ManagementClient through to MessagePump Fix Queue model definition
Determine whether policy override of delivery limit is supported by the broker version
…itMQ management client
…ensure we get complete queue object
| @@ -0,0 +1,182 @@ | |||
| #nullable enable | |||
|
|
|||
| namespace NServiceBus.Transport.RabbitMQ.Administration; | |||
There was a problem hiding this comment.
| namespace NServiceBus.Transport.RabbitMQ.Administration; | |
| namespace NServiceBus.Transport.RabbitMQ; |
| using System.Threading.Tasks; | ||
| using System.Threading; | ||
|
|
||
| interface IBrokerVerifier |
There was a problem hiding this comment.
I don't really see the benefit in having this interface. Is there a specific reason to have it?
| @@ -0,0 +1,34 @@ | |||
| #nullable enable | |||
|
|
|||
| namespace NServiceBus.Transport.RabbitMQ.Administration.ManagementClient.Converters; | |||
There was a problem hiding this comment.
Let's change all of the management client namespaces to
| namespace NServiceBus.Transport.RabbitMQ.Administration.ManagementClient.Converters; | |
| namespace NServiceBus.Transport.RabbitMQ.ManagementClient; |
| using System.Threading.Tasks; | ||
| using NServiceBus.Transport.RabbitMQ.Administration.ManagementClient.Models; | ||
|
|
||
| interface IManagementClient |
There was a problem hiding this comment.
Do we really need this interface?
|
|
||
| namespace NServiceBus.Transport.RabbitMQ.Administration.ManagementClient; | ||
|
|
||
| interface IManagementClientFactory |
There was a problem hiding this comment.
Same question, why the interface?
| @@ -0,0 +1,15 @@ | |||
| namespace NServiceBus.Transport.RabbitMQ.Administration.ManagementClient.Models; | |||
|
|
|||
| /// <summary> | |||
There was a problem hiding this comment.
Same comment about xml comments.
| @@ -9,8 +9,10 @@ | |||
| class ConnectionConfiguration | |||
| { | |||
| const bool defaultUseTls = false; | |||
| const int defaultPort = 5672; | |||
| const int defaultTlsPort = 5671; | |||
| const int defaultBrokerPort = 5672; | |||
There was a problem hiding this comment.
I don't think any of the changes in this file should be done. Instead, the management client should just take a regular ConnectionConfiguration instance. The knowledge of needing to override the ports for the management client should be there instead.
| @@ -96,6 +96,36 @@ public static TransportExtensions<RabbitMQTransport> ConnectionString(this Trans | |||
| return transportExtensions; | |||
| } | |||
|
|
|||
| /// <summary> | |||
There was a problem hiding this comment.
Looks like also need a way to disable the management client usage through the legacy config API.
|
|
||
| ManagementConnectionConfiguration = string.IsNullOrEmpty(managementConnectionString) ? | ||
| ConnectionConfiguration.ConvertToManagementConnection(BrokerConnectionConfiguration) : | ||
| ConnectionConfiguration.Create(managementConnectionString, isManagementConnection: true); |
There was a problem hiding this comment.
Oh, yeah my comment about not using ConnectionConfiguration above... yeah that needs to change.
The management client connection string is going to be an http/s URL, which isn't something ConnectionConfiguration knows how to parse, so we should never be trying to pass that in.
ConnectionConfiguration is for parsing our own made up connection string format and the amqp format that the client understands.
| /// <param name="enableDelayedDelivery">Should the delayed delivery infrastructure be created by the endpoint</param> | ||
| public RabbitMQTransport(RoutingTopology routingTopology, string connectionString, bool enableDelayedDelivery) | ||
| public RabbitMQTransport(RoutingTopology routingTopology, string connectionString, bool enableDelayedDelivery, string managementConnectionString = null) |
There was a problem hiding this comment.
We should not use optional parameters in public APIs. They can be hard to version properly. Instead, we should add overloads that take the management connection string.
…gs and creating a policy
Simplified namespaces
…verload where parameter is required
No description provided.