Upgrade to core V8 #720

SzymonPobiega · 2021-01-11T12:51:41Z

No description provided.

src/NServiceBus.Transport.RabbitMQ.AcceptanceTests/ConfigureEndpointRabbitMQTransport.cs

src/NServiceBus.Transport.RabbitMQ.AcceptanceTests/When_using_a_custom_message_id_strategy.cs

src/NServiceBus.Transport.RabbitMQ.Tests/ApprovalFiles/APIApprovals.Approve.approved.txt

...ServiceBus.Transport.RabbitMQ.Tests/ConnectionString/ConnectionConfigurationWithAmqpTests.cs

src/NServiceBus.Transport.RabbitMQ.Tests/RabbitMqContext.cs

src/NServiceBus.Transport.RabbitMQ/RabbitMQTransport.cs

src/NServiceBus.Transport.RabbitMQ.AcceptanceTests/ConfigurationHelpers.cs

timbussmann · 2021-02-02T12:32:32Z

src/NServiceBus.Transport.RabbitMQ.Tests/ApprovalFiles/APIApprovals.Approve.approved.txt

    }
    public static class RabbitMQTransportOptionsExtensions
    {
        public static void UseNonPersistentDeliveryMode(this NServiceBus.PublishOptions options) { }
        public static void UseNonPersistentDeliveryMode(this NServiceBus.ReplyOptions options) { }
        public static void UseNonPersistentDeliveryMode(this NServiceBus.SendOptions options) { }
    }
-    public static class RabbitMQTransportSettingsExtensions


shouldn't those be properly obsoleted?

src/NServiceBus.Transport.RabbitMQ/Configuration/AmqpConnectionString.cs

src/NServiceBus.Transport.RabbitMQ/Connection/ConnectionFactory.cs

timbussmann · 2021-02-02T12:50:41Z

src/NServiceBus.Transport.RabbitMQ/RabbitMQTransport.cs

+        /// <summary>
+        /// Specifies if an external authentication mechanism should be used for client authentication.
+        /// </summary>
+        public bool UseExternalAuthMechanism { get; set; } = false;


there seems to be an inconsistency whether the default value is specified or not (I think it would make sense to always explicitly specify it).

Not sure I understand. You mean I should always specifiy default value for bool properties? I think this is just by accident.

src/NServiceBus.Transport.RabbitMQ/RabbitMQTransport.cs

src/NServiceBus.Transport.RabbitMQ/Receiving/MessagePump.cs

src/NServiceBus.Transport.RabbitMQ/obsoletes-v8.cs

Bring back UseTLS setting

bording

There is a lot of formatting cleanup that also need to be done, but I didn't call that out specifically anywhere.

Overall, I can follow the reasoning behind some/most of the changes, but the configuration stuff is a big change that I'm not sure I understand the point of just yet.

My initial reaction here is that we've made the configuration code much worse, and the experience is also going to be worse for people using the transport.

bording · 2021-02-10T19:14:48Z

src/NServiceBus.Transport.RabbitMQ/Sending/BasicPropertiesExtensions.cs


                if (delay > DelayInfrastructure.MaxDelayInSeconds)
                {
-                    throw new Exception($"Message cannot be sent with {nameof(DoNotDeliverBefore)} value '{doNotDeliverBefore.At}' because it exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");
+                    throw new Exception($"Message cannot set to be delivered at '{dispatchProperties.DoNotDeliverBefore.At}' because the delay exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");


This error message currently isn't a valid sentence. Probably needs something like:

Suggested change

throw new Exception($"Message cannot set to be delivered at '{dispatchProperties.DoNotDeliverBefore.At}' because the delay exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");

throw new Exception($"Message cannot be delivered at '{dispatchProperties.DoNotDeliverBefore.At}' because the delay exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");

However, I prefer the previous format for the error message. With the current change, it doesn't make it clear that the error is happening because of an invalid DoNotDeliverBefore on the message. You're now asking them to see a value, and then figure out where the value came from.

bording · 2021-02-10T19:15:26Z

src/NServiceBus.Transport.RabbitMQ/Sending/BasicPropertiesExtensions.cs


                if (delay > DelayInfrastructure.MaxDelayInSeconds)
                {
-                    throw new Exception($"Message cannot be sent with {nameof(DelayDeliveryWith)} value '{delayDeliveryWith.Delay}' because it exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");
+                    throw new Exception($"Message cannot be delayed by '{dispatchProperties.DelayDeliveryWith.Delay}' because it exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");


Same comment about this error message. I would prefer to retain the {nameof(DelayDeliveryWith)} in some way.

bording · 2021-02-10T19:26:03Z

src/NServiceBus.Transport.RabbitMQ/Sending/BasicPropertiesExtensions.cs

@@ -32,15 +29,15 @@ public static void Fill(this IBasicProperties properties, OutgoingMessage messag
                properties.Headers[DelayInfrastructure.DelayHeader] = Convert.ToInt32(delay);
            }

-            if (deliveryConstraints.TryGet(out DiscardIfNotReceivedBefore timeToBeReceived) && timeToBeReceived.MaxTime < TimeSpan.MaxValue)
+            if (dispatchProperties.DiscardIfNotReceivedBefore != null && dispatchProperties.DiscardIfNotReceivedBefore.MaxTime < TimeSpan.MaxValue)


Suggested change

if (dispatchProperties.DiscardIfNotReceivedBefore != null && dispatchProperties.DiscardIfNotReceivedBefore.MaxTime < TimeSpan.MaxValue)

if (dispatchProperties.DiscardIfNotReceivedBefore?.MaxTime < TimeSpan.MaxValue)

bording · 2021-02-10T19:36:22Z

src/NServiceBus.Transport.RabbitMQ/Routing/DirectRoutingTopology.cs

@@ -8,40 +10,93 @@
    /// <summary>
    /// Route using a static routing convention for routing messages from publishers to subscribers using routing keys.
    /// </summary>
-    class DirectRoutingTopology : IRoutingTopology
+    public class DirectRoutingTopology : IRoutingTopology


At the point where I'm writing this comment, I haven't seen the configuration changes yet, but I'm taking an educated guess at how they've changed if you've made this public.

I'm not a fan of changes that make these topology types public.

The only alternative would be to use enums but this makes is awkward when combined with a custom topology because that would have to be an instance.

In alternative could be some static field e.g. have Topologies static class with fields for direct and conventional topology. Then the user could do

config.RoutingTopology = Topologies.Direct; config.RoutingTopology = Topologies.Conventional; config.RoutingTopology = new MyCustomTopology(); config.RoutingTopology = new ConventionalRoutingTopology(false, oneConvention, anotherConvention)

but since the most common case of conventional topology is covered by default settings, I think that would be excessive complexity. If a user knows what direct topology is, they can surely be bothered by instantiating a public class.

Hard disagree here. If we are "improving" the transport seam, why is leading to APIs that are worse than what we had before?

I disagree that this is worse if you take into account all the usage scenarios. The advantage of the new API is that is allows users to inspect the state of the configuration object.

bording · 2021-02-10T19:37:06Z

src/NServiceBus.Transport.RabbitMQ/Routing/ConventionalRoutingTopology.cs

@@ -21,55 +23,101 @@
    /// <item><description>we generate an exchange for each queue so that we can do direct sends to the queue. it is bound as a fanout exchange</description></item>
    /// </list>
    /// </summary>
-    class ConventionalRoutingTopology : IRoutingTopology
+    public class ConventionalRoutingTopology : IRoutingTopology


Feels like a mistake to be making these public.

bording · 2021-02-10T19:41:41Z

src/NServiceBus.Transport.RabbitMQ/Routing/ConventionalRoutingTopology.cs

-                type = typeof(object);
-            }
+            // Make handlers for IEvent handle all events whether they extend IEvent or not
+            var typeToSubscribe = type.MessageType != typeof(IEvent) ? type.MessageType : typeof(object);


This is a somewhat questionable legacy thing to do, and is only still here because changing it would be wire compat break. That being said, I'd prefer:

Suggested change

var typeToSubscribe = type.MessageType != typeof(IEvent) ? type.MessageType : typeof(object);

var typeToSubscribe = type.MessageType == typeof(IEvent) ? typeof(object) : type.MessageType;

bording · 2021-02-10T20:05:13Z

src/NServiceBus.Transport.RabbitMQ/Receiving/MessagePump.cs

+
+            if (settings.UsePublishSubscribe)
+            {
+                Subscriptions = new SubscriptionManager(connectionFactory, routingTopology, settings.ReceiveAddress);


If the thing that creates a message pump has access to all the things required to create a SubscriptionManager it seems better to me for it to create one if needed instead. That means you can stop passing in routingTopology just to be able to create this.

Good point. I'll discuss that with other TF members to make this consistent across transports.

bording · 2021-02-10T20:30:08Z

src/NServiceBus.Transport.RabbitMQ/RabbitMQTransport.cs

+        /// <summary>
+        /// The routing topology to use. If not set the conventional routing topology will be used <seealso cref="ConventionalRoutingTopology"/>.
+        /// </summary>
+        public IRoutingTopology RoutingTopology


Ok, yeah this is what I afraid of when I saw that the topology classes had been made public.. This is a strictly worse configuration experience compared to what we had before. This puts the onus on the caller to have to create a topology class in all instances of using this API.

In the past that would only have been required when they had created their own custom topology implementation.

They only need to do it if they chose to select a non-default topology. In this version the conventional topology is made the default. Since very few users use the direct one, I don't this is going to be a major issue for the majority.

bording · 2021-02-10T20:34:08Z

src/NServiceBus.Transport.RabbitMQ/Configuration/AmqpConnectionString.cs

+{
+    class AmqpConnectionString
+    {
+        public static Action<RabbitMQTransport> Parse(string connectionString)


This is parsing a connection string. The idea that this is returning an entire RabbitMQTransport instance to do that seems like a major smell to me.

We used to have a ConnectionConfiguration class for a reason.

It does not return the instance but rather an action that configures that instance. Plus the RabbitMQTransport is now a configuration object only.

bording · 2021-02-10T20:36:02Z

src/NServiceBus.Transport.RabbitMQ/RabbitMQTransport.cs

+        {
+            if (connectionString.StartsWith("amqp", StringComparison.OrdinalIgnoreCase))
+            {
+                AmqpConnectionString.Parse(connectionString)(this);


Having to call Parse and then cast like that? This is not something that should be required to do. Why not keep the ConnectionConfiguration class and not have all this going on?

case? This is passing the current instance to the configuration action. Why having two classes with exactly same properties, one public and one private?

- Improve config API

SzymonPobiega · 2021-02-17T11:09:36Z

@bording we discussed the transport config APIs during today's sync and we decided to:

revert making conventional the default topology. In order to enforce the choice of topology we added it as a constructor parameter. Topologies are still public in case someone wants to created them with non-default arguments
the exchangeNameConvention parameter to ConventionalRoutingTopology is going to continue to be there as it is useful in case of events being defined as nested classes. It is now not going to be exposed for the non-advanced users so I don't see any harm in exposing it
the prefetch count is now a custom delegate so that the user knows that the argument is and what they should return
the connection string has been made mandatory
the NServiceBus connection string is not removed because we don't feel like changes required to SC to make it work can be done in scope of this TF
the certificate stays are property. We feel like having the user new up an instance of X509Certificante is not a deal breaker for our users.

This is definitely not the last TF that is going to be run in scope of the V8 so there is no problem with DevExp making the APIs even more polished. We wanted to focus on getting them consistent while not blocking any existing use cases.

bording · 2021-02-17T17:57:40Z

@SzymonPobiega Reading your response, I'll admit to be being rather disheartened, because I don't really see how much of that addresses the concerns I raised.

the exchangeNameConvention parameter to ConventionalRoutingTopology is going to continue to be there as it is useful in case of events being defined as nested classes. It is now not going to be exposed for the non-advanced users so I don't see any harm in exposing it

This one specifically I really disagree with. There's basically almost nothing you can do with that parameter that leaves you with a correct working system. The only thing you could do would be to use FullName instead. If you want to enable that, there should just be a bool parameter you can set to opt in to set that behavior.

SzymonPobiega · 2021-02-22T10:50:47Z

@bording I've removed the additional parameter. Should be now good to go.

Need to push the package to myget for smoke testing

SzymonPobiega · 2021-02-23T08:35:53Z

@bording I had to merge it to be able to smoke test the transports. Any further adjustments to the API are going to be done by subsequent task forces, either in Architecture or DevExperience.

SzymonPobiega requested review from tmasternak, mauroservienti and timbussmann January 11, 2021 12:51

SzymonPobiega self-assigned this Jan 11, 2021

tmasternak requested changes Feb 1, 2021

View reviewed changes

timbussmann reviewed Feb 2, 2021

View reviewed changes

SzymonPobiega force-pushed the upgrade-to-core-8 branch from 6272e25 to 2b009da Compare February 10, 2021 17:07

Update core V8

175c297

Bring back UseTLS setting

SzymonPobiega force-pushed the upgrade-to-core-8 branch from 2b009da to 175c297 Compare February 10, 2021 17:09

bording previously requested changes Feb 10, 2021

View reviewed changes

SzymonPobiega added 3 commits February 11, 2021 11:37

Fix formatting and error messages.

2ac3d63

Upgrade to core alpha 819

5147313

Fix path-based tests on linux

278d119

tmasternak approved these changes Feb 11, 2021

View reviewed changes

SzymonPobiega added 3 commits February 11, 2021 14:03

Another attempt to fix the tests

a3dc272

Dispose receivers when shutting down infra

21e1aff

- Update to 842

994014e

- Improve config API

Fix test

88abf19

SzymonPobiega added 2 commits February 22, 2021 11:22

Remove exchangeNameConvention from ConventionalRoutingTopology

30fa65d

Approve API

e3dc71b

SzymonPobiega merged commit 241f98d into master Feb 23, 2021

SzymonPobiega deleted the upgrade-to-core-8 branch February 23, 2021 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to core V8 #720

Upgrade to core V8 #720

SzymonPobiega commented Jan 11, 2021

timbussmann Feb 2, 2021

timbussmann Feb 2, 2021

SzymonPobiega Feb 3, 2021

bording left a comment

bording Feb 10, 2021

SzymonPobiega Feb 11, 2021

bording Feb 10, 2021

SzymonPobiega Feb 11, 2021

bording Feb 10, 2021

bording Feb 10, 2021

SzymonPobiega Feb 11, 2021

bording Feb 11, 2021

SzymonPobiega Feb 11, 2021

bording Feb 10, 2021

bording Feb 10, 2021

bording Feb 10, 2021

SzymonPobiega Feb 11, 2021

bording Feb 10, 2021

SzymonPobiega Feb 11, 2021

bording Feb 10, 2021

SzymonPobiega Feb 11, 2021

bording Feb 10, 2021

SzymonPobiega Feb 11, 2021

SzymonPobiega commented Feb 17, 2021

bording commented Feb 17, 2021 •

edited

Loading

SzymonPobiega commented Feb 22, 2021

SzymonPobiega commented Feb 23, 2021

	throw new Exception($"Message cannot set to be delivered at '{dispatchProperties.DoNotDeliverBefore.At}' because the delay exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");
	throw new Exception($"Message cannot be delivered at '{dispatchProperties.DoNotDeliverBefore.At}' because the delay exceeds the maximum delay value '{TimeSpan.FromSeconds(DelayInfrastructure.MaxDelayInSeconds)}'.");

	if (dispatchProperties.DiscardIfNotReceivedBefore != null && dispatchProperties.DiscardIfNotReceivedBefore.MaxTime < TimeSpan.MaxValue)
	if (dispatchProperties.DiscardIfNotReceivedBefore?.MaxTime < TimeSpan.MaxValue)

	var typeToSubscribe = type.MessageType != typeof(IEvent) ? type.MessageType : typeof(object);
	var typeToSubscribe = type.MessageType == typeof(IEvent) ? typeof(object) : type.MessageType;

Upgrade to core V8 #720

Upgrade to core V8 #720

Conversation

SzymonPobiega commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bording 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

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

SzymonPobiega commented Feb 17, 2021

bording commented Feb 17, 2021 • edited Loading

SzymonPobiega commented Feb 22, 2021

SzymonPobiega commented Feb 23, 2021

bording commented Feb 17, 2021 •

edited

Loading