Nservicebus: Routing API improvements

[ramonsmits]

After trying to understand the routing API I have some suggestions for improvement

This issue describes the following improvements

• Use of IEnumerable instead or EndpointInstance[] array
• EndpointName as string
• Improve discovery of AtMachine
• RouteToEndpoint duplication

Use of IEnumerable instead or EndpointInstance[] array

Change this to allow passing of IEnumerable<EndpointInstance>

Current

``` c#
public void Add(EndpointName endpoint, params EndpointInstance[] instances);

#### Proposal

``` c#
public void Add(EndpointName endpoint, IEnumerable<EndpointInstance> instances);

EndpointName as string

Why not be able to pass in a string instead of EndpointName? If EndpoinName is needed then consider an implicit string conversion.

Not needing to pass endpoint multiple times:

Current

``` c#
routing.Mapping.Physical.Add(endpoint, new EndpointInstance(endpoint).AtMachine("A"));

#### Proposal

Endpoint only in EndpointInstance constructor

``` c#
routing.Mapping.Physical.Add(new EndpointInstance(endpoint).AtMachine("A"));

Question: What happens if I create a new EndpointName object for all instance objects?

or

Endpoint in the Add, not needed in the EndpointInstance object.

``` c#
routing.Mapping.Physical.Add(endpoint, new EndpointInstance().AtMachine("A"));

## Improve discovery of AtMachine

Discovery of `AtMachine` is hard, its hidden. The API should force the user to specify the machine name.
#### Current:

``` c#
routing.Mapping.Physical.Add(endpoint, new EndpointInstance(endpoint).AtMachine("Machine-A"), new EndpointInstance(endpoint).AtMachine("Machine-B"));

Proposal

Create a _MsmqEndpointInstance_, basically making EndpointInstance an interface or abstract class.

``` c#
routing.Mapping.Physical.Add(.., new MsmqEndpointInstance(endpoint,"Machine-A"));

Have transport specific overloads that _must_ be used when adding entries to the physical collection.

``` c#
routing.Mapping.Physical.AddMsmqInstance(.., new EndpointInstance(endpoint), "Machine-A");

or

Have a transport specific property on the physical model that provides transport specific overloads.

``` c#
routing.Mapping.Physical.Msmq.Add(.., new EndpointInstance(endpoint), "Machine-A");

## RouteToEndpoint duplication

This method is present on which is confusing and even with the same signature which is confusing.
#### Current

``` c#
routing.RouteToEndpoint();
routing.Mapping.Logical.RouteToEndpoint();

Proposal

Remove the overload on the UnicastRoutingSettings, removing this _convenience_ method makes the api more clear.

An alternative is to rename the method

[timbussmann]

Hm I think I prefer the params API for Add better.

:+1: to the improvements around EndpointName

[andreasohlund]

@SzymonPobiega @indualagarsamy @janovesk since you all where part of the routing TF it would be great if you can chime in?

[SzymonPobiega]

My thoughts

Use of IEnumerable instead or EndpointInstance[] array

:+1: :-1: I think both make sense although I don't have a strong opinion here.

EndpointName as string

:+1: Remove it from all the user-facing API calls.

No need to pass EndpointName when passing a collection of instance names

:+1: Each EndpointInstance carries information about the endpoint name so passing it in the parameter is redundant.

Improve discovery of AtMachine

:-1: This is a very, very specific API. As such, I prefer to keep is clean and focused on the expense of being somewhat less discoverable. EndpointInstance has properties and the cleanest (IMO) way of setting them is via extension methods on the instance object itself. Again, this is an API most users would not use frequently.

RouteToEndpoint duplication

:-1: The convenience method has been added after an API review call because it is expected to cover vast majority of cases. On the other hand, for advanced usages (e.g. extensibility) it makes sense to expose the full API in the settings object because this object can be easily accessed from a feature (or a startup task).

[andreasohlund]

I'm happy with doing the proposal from @SzymonPobiega within the v6 scope. @ramonsmits ok?

[tmasternak]

ping @ramonsmits

[danielmarbach]

@SzymonPobiega would it make sense to incorporate suggestions into https://github.com/Particular/PlatformDevelopment/issues/814 and close this one?

[SzymonPobiega]

This issue is linked to 814. When we start working on it tomorrow I plan to actually extract all the info from the linked issues as this one and copy over to 814 and close the original issues.

[ramonsmits]

Reason for the IEnumerable is that you can pass it a lot of collection types. Not only just an array or making use of the params approach.

[SimonCropp]

This makes more sense when the instacne of EndpointInstance will be known at compile time. ie they are passing in a set known number of EndpointInstance using compile code

public void Add(EndpointName endpoint, params EndpointInstance[] instances);

This makes more sens if they will be querying some external source to get a runtime list of EndpointInstances

public void Add(EndpointName endpoint, IEnumerable<EndpointInstance> instances);

[ramonsmits]

@SzymonPobiega Does it matter if Machine is very specific or not? I don't even understand what you mean by that.

For me its about that it was not easy to spot / configure. Not being self explanatory when using MSMQ. I needed two zoom calls of which the second was with the person that made it :-). Otherwise, it is too complex and this is about removing complexity, making it more discoverable for the enduser.

[ramonsmits]

This makes more sens if they will be querying some external source to get a runtime list of EndpointInstances

@SimonCropp Which is pretty likely when doing your own custom routing from within code.

[SzymonPobiega]

@ramonsmits I don't see how MsmqEndpointInstance is more discoverable than .AtMachine. The signature says EndpointInstance. I don't think a standard user would think that there might be a subclass specific for MSMQ he needs to use.

Using EndpointInstance without .AtMachine will result in using a queue on the local machine. I think this is a reasonable default equivalent to using a queue name without the @ sign.

[ramonsmits]

@SzymonPobiega The endpoint instance would be either an interface or base class with a protected constructor.

[SzymonPobiega]

Then each transport would have to have its own (identical) implementation. Sounds like an overkill.

[ramonsmits]

@SzymonPobiega Maybe, I'm not familiar enough. I just mentioned it as a possible option to make machine name more discoverable or enforce it.

[distantcam]

Re: IEnumerable vs params EndpointInstance[] - Why can't we have both?

[distantcam]

Regarding AtMachine

What happens if the endpoint is not a MSMQ specific endpoint? Should AtMachine still work or throw an exception? Can we even tell if an EndpointInstance is MSMQ specific? Do we care?

Either we don't care that specific transport info (like AtMachine) are applied to non-msmq endpoints, or we should have specific endpoint type subclasses (like MSMQEndpointInstance).

I don't see how MsmqEndpointInstance is more discoverable than .AtMachine. The signature says EndpointInstance. I don't think a standard user would think that there might be a subclass specific for MSMQ he needs to use.

If EndpointInstance is made abstract then users would go looking for specific implementations. I think that makes sense since it's hard to think about an endpoint instance without knowing what transport it's using.

Also why is EndpointInstance sealed?

[timbussmann]

Re: IEnumerable vs params EndpointInstance[] - Why can't we have both?

Not sure, but wouldn't that result in some pains because of ambiguous parameters?

What happens if the endpoint is not a MSMQ specific endpoint? Should AtMachine still work or throw an exception?

I don't think we should throw an exception. Other transports should just ignore that information. That would be solved anyway when we move MSMQ out of the core?

[SzymonPobiega]

The main problem with an abstract class I see is that all the other transports would have to have an empty implementation of their endpoint instance. That's why we used an extension method originally.

Regarding AtMachine and throwing exceptions in other transports, each TransportDefinition has to provide a method for translating EndpointInstance (or its closely related LogicalAddress) to a physical queue. This is the place where an exception should be thrown if there is a property that does not apply to a given transport.

That said, I don't believe any transport does this validation.

[distantcam]

Re: IEnumerable vs params EndpointInstance[] - Why can't we have both?
Not sure, but wouldn't that result in some pains because of ambiguous parameters?

I don't believe so. I've implemented both and the tests are fine.

[timbussmann]

I don't believe so. I've implemented both and the tests are fine.

if you tested it and it doesn't show up as an issue, then I'm :+1: on providing both.

[danielmarbach]

A discussion from the past

https://github.com/Particular/NServiceBus/pull/2280#issuecomment-122260403

Might help to make up your minds

[distantcam]

@danielmarbach Hmmm, ignoring a false positive in R# analyzer (that I don't have installed anyway) vs calling ToArray which does a bunch of memory allocs. I know which one I prefer. ;)

[andreasohlund]

@timbussmann pretty sure we can close this?

[timbussmann]

yep I agree. The APIs in the issue description have been reworked before the v6 release.