Nservicebus: DeliveryConstraint what are these for ?

[johnsimons]

Also it seems as part of implementing a transport I have to return DeliveryConstraints.

``` c#
public class RabbitMQTransport : TransportDefinition
{
public override IEnumerable GetSupportedDeliveryConstraints()
{
// ???
}
...
}

But the api tells me to return a `Type`, shouldn't this API be more restrictive ?

Something like:

``` c#
public override IEnumerable<DeliveryConstraints> GetSupportedDeliveryConstraints()
{
             ???
}

still very confusing to use this api because eg if i want to return DiscardIfNotReceivedBefore, I would need to new it up and set the maxtime, eg:

c# public override IEnumerable<DeliveryConstraints> GetSupportedDeliveryConstraints() { // But where do I get the maxTime from ? This would be specified by a user right ? So I guess I need access to the context? yield return new DiscardIfNotReceivedBefore(maxTime); }

I am super confused, please enlight me ?

[andreasohlund]

You need to return the list of delivery constraints the transport supports. Agree that the API isn't super good but I think we have bigger fish to fry atm?

[johnsimons]

So what are the possible constraints ?

And what are the RabbitMQ constraints ?

Are these constraints declared for each transport ?

What are these constraints used for ?

I really need more guindance here

[johnsimons]

@SzymonPobiega @janovesk help please ?

[mikeminutillo]

Looking into it, it seems like the transport declares which types of Delivery Constraints it can handle (in the TransportDefinition.GetSupportedDeliveryConstraints()). Then the user can specify delivery constraints when they send (or publish) a message. These are sent with the DeliveryOptions as a part of the TransportOperation that gets passed in to IDispatchMessages.Dispatch(...).

During dispatch, it is up to the transport to downcast each of the DeliveryConstraints instances into their most concrete forms and change the way they perform the dispatch operation to match the constraints.

Is that all correct?

If so, does the core check to ensure that we don't send anything to the transport that it can't handle?

[SzymonPobiega]

Probably @andreasohlund can help.

[timbussmann]

so is the idea behind DeliveryConstraint to allow transport to bring their own constraints?

[andreasohlund]

I'd say its about the transports telling the core what constraints it
supports

On Wed, Dec 16, 2015 at 12:04 PM, Tim Bussmann [email protected]
wrote:

so is the idea behind DeliveryConstraint to allow transport to bring
their own constraints?

—
Reply to this email directly or view it on GitHub
https://github.com/Particular/NServiceBus/issues/3226#issuecomment-165067825
.

[andreasohlund]

@johnsimons proposed to convert the constraints to an enum instead, I like that

[johnsimons]

Also, see @mikeminutillo point regarding the framework validating on the send, is this something we also want ?

[johnsimons]

@timbussmann @danielmarbach @SzymonPobiega are you guys ok with making constraints a multi flag enum ?

[mikeminutillo]

I'm not sure I like the separation between declaring what constraints we support and actually supporting them. It means there's two places to update when you want to add support for a delivery constraint and if you forget one of them, it won't be immediately apparent (especially as we don't verify that all constraints sent to the transport are supported). What about something like this:

internal interface IDeliveryConstraintHandler
{
    void HandleConstraint(DeliveryConstraint constraint, ReadOnlyContextBag contextBag);
}

abstract class DeliveryConstraintHandler<TConstraint> : IDeliveryConstraintHandler
    where TConstraint : DeliveryConstraint
{
    protected abstract void Handle(TConstraint constraint, ReadOnlyContextBag contextBag);

    void IDeliveryConstraintHandler.HandleConstraint(DeliveryConstraint constraint, ReadOnlyContextBag contextBag)
    {
        Handle(constraint as TConstraint, contextBag);
    }
}

class TransportDeliveryConstraintSupport
{
    IDictionary<Type, IDeliveryConstraintHandler> _handlers = new Dictionary<Type, IDeliveryConstraintHandler>();

    public void Add<TConstraint>(DeliveryConstraintHandler<TConstraint> handler) where TConstraint : DeliveryConstraint
    {
        _handlers[typeof(TConstraint)] = handler;
    }

    public IEnumerable<Type> GetSupportedConstraints()
    {
        return _handlers.Keys;
    }

    public void Apply(IEnumerable<DeliveryConstraint> constraints, ReadOnlyContextBag contextBag)
    {
        foreach(var constraint in constraints)
        {
            IDeliveryConstraintHandler handler;
            if(_handlers.TryGetValue(constraint.GetType(), out handler))
            {
                handler.HandleConstraint(constraint, contextBag);
            }
        }
    }
}

Then during Init we'd create and return one of these:

var deliveryConstraintSupport = new TransportDeliveryConstraintSupport();
deliveryConstraintSupport.Add(new DiscardIfNotDeliveredBeforeHandler());

If you want to know what constraints the transport supports use deliveryConstraintSupport.GetSupportedConstraints()

And when we want to dispatch a message we'd do this (this is RabbitMq specific):

var properties = channel.CreateBasicProperties();

var bag = new ContextBag();
bag.Set<IBasicProperties>(properties);
deliveryConstraintSupport.Apply(constraints, bag);

And for completeness...

class DiscardIfNotDeliveredBeforeHandler : DeliveryConstraintHandler<DiscardIfNotDeliveredBefore>
{
    protected override void Handle(DiscardIfNotDeliveredBefore constraint, ReadOnlyContextBag contextBag)
    {
        if(constraint.MaxTime < TimeSpan.MaxValue)
        {
            var properties = contextBag.Get<IBasicProperties>();
            properties.Expiration = constraint.MaxTime.TotalMilliseconds.ToString(CultureInfo.InvariantCulture);
        }
    }
}

[danielmarbach]

@mikeminutillo I like this proposal. We could still keep the class and transports could extend it but it makes it more explicit. We would need to use different vocabulary than handler maybe to not confuse users

[danielmarbach]

I removed the milestone for v6. If you feel strongly about it mark it as Consider for v6.

[andreasohlund]

We currently have 4 constraints:

• DelayedDelivery+DeliverAt: Used by the TM. We want to push this down into the transports which will means that they will go away. Not sure if we have an issue for this yet
• NonDurableDelivery: This is very transport specific so I'd propose we push this down into the transports as well. Ie each transport can provide attribute/conventions/etc to expose what they support. Eg MSMQ would have [Express] and rabbit would have [NonDurable] , sql would not provide this at all. In short: This one goes away
• TimeToBeReceived: We either push this one down as well or create a specific api for this?

Summary: Lets focus on 🔥 the delivery constraints instead?

[danielmarbach]

If we do that I would prefer a less allocation, inheritance heavy approach. So I'm for burning these delivery constraints if we manage to come up with the transport maintainers with a more intention revealing approach. The benefit of the current design is that they are exposed on the transport operations and "discoverable"

[andreasohlund]

So lets make sure we have issues to track the action point mentioned and then close? (I can handle that)

[danielmarbach]

I'm closing this discussion. The initial question has been answered. @andreasohlund Let's have a discussion when you are back about the current state of delivery constraints in the sync. I doubt we want to break the transport seam yet again for something of "little value" like the delivery constraints. Happy to be convinced otherwise though.