Introduce IBasicPublishBatch

[kjnilsson]

I've changed @YulerB 's BatchMessage for an IBasicPublishBatch approach with an Add method with a similar signature to IModel.BasicPublish. This allows messages for arbitrary exchanges and routing keys to be grouped together in a single write(2) to the socket.

Writing in larger batches ensures tcp segments are better utilised as well as reduces the number of tcp acks that need to be issued. This yields fairly dramatic throughput improvements outlined here

@bording @YulerB @michaelklishin - WDYT?

Example:

        let mb = model.CreateBasicPublishBatch()
        mb.Add("exch", "key", false, basicProps, data)
        mb.Publish()

[YulerB]

I would love an overload called AddRange that took an IEnumerable of Message that contained these elements.

[kjnilsson]

That's something that could be very easily added as an application specific extension method as required. From a core api point of view I don't see the need to introduce a Message type to hold the fields in question (If I understood your request correctly).

that said maybe the name IMessageBatch isn't that great - IBasicPublishBatch feels more appropriate perhaps.

[YulerB]

Should we clear commands to allow reuse of the batch , or do you see this as a way of issuing a batch of commands daily, like an actor could?

[bording]

I believe the intention is that you'll be creating a new instance via CreateBasicPublishBatch() and not reusing these because the constructor is internal.

[kjnilsson]

I've deliberately not put any particular intent on the API but just kept it as a simple buffer abstraction that users can use as they see fit.

[bording]

@kjnilsson 👍 For the API changes here.

I'm still not sure I can use it with NServiceBus because of how I have to correlate publisher confirm acks back to tasks awaiting completion, but I'll have to give that some more thought.

[kjnilsson]

@bording Thanks for reviewing.
I would have hoped that these changes wouldn't change the semantics of publishing in any way and that confirms would flow back in a similar manner as if the messages were published one after the other (allowing for some timing related differences) but maybe the there are some further subtleties in how you implemented batching in NSB.

[YulerB]

@kjnilsson 👍 For the API changes here also.

[bording]

I would have hoped that these changes wouldn't change the semantics of publishing in any way and that confirms would flow back in a similar manner as if the messages were published one after the other (allowing for some timing related differences) but maybe the there are some further subtleties in how you implemented batching in NSB.

@kjnilsson Currently there is a 1-to-1 mapping between each message sent and the API call to send it, so I can create a TaskCompletionSource for each message that is published, storing them in a dictionary with the sequence number as the key. That lets me associate each publisher confirm ack back to each entry in the dictionary (with some special handling when a multiple ack comes back).If I get a batch of messages, I'm able to return use Task.WhenAll to create a single task that completes when all of the messages in the batch receive acks.

If I use this new API, I no longer have an API call per message, so being able to wait for all of the individual acks before completing the operation becomes trickier to manage.

This PR probably isn't the correct place to go into more detail, but if you want to know more, I'd be happy to walk you through it.

[kjnilsson]

Maybe you could grab the NextPublishSeqNo before and after batch Publish and build up the list of TCSs from that, not sure if that fits.

[bording]

Maybe you could grab the NextPublishSeqNo before and after batch Publish and build up the list of TCSs from that, not sure if that fits.

That was something I was thinking about, but then I'd have to add another Task.WhenAll in there to combine them back into a single task that I can return. That adds more overhead/allocations, so I'll have to test to see how it all balances out.

[kjnilsson]

@michaelklishin @lukebakken I think we're there with this PR unless either of you would like any changes?

[michaelklishin]

Not sure when I'd get a chance to take a look this week but I remember that this needs reviewing. Thank you for the reminder, though 👍

[danielmarbach]

While this is better it is also a breaking change, was this intentional?

[kjnilsson]

It was but given how often IFrameHandler appears in (unnecessarily) public APIs we should probably revert it. :/

[danielmarbach]

While this is better it is also a breaking change, was this intentional?

[kjnilsson]

There is a pre-release available now that contain this feature: https://www.nuget.org/packages/RabbitMQ.Client/5.1.0-pre2

[cjlotz]

With the new BatchPublish behaviour, what is the expected behaviour w.r.t to publisher confirmations? Is the expectation to get a single confirmation if the complete batch of messages were all sent successfully or do we get an individual confirmation for each message in the batch? From testing it seems that you only receive a single confirmation with the Multiple flag set to true? Any more documentation available on the topic?

[michaelklishin]

Please direct your questions to RabbitMQ users. Publishing a batch of messages is not semantically different from publishing them one by one since there is no batch publish operation in the protocol. This PR batches socket writes.

[salixzs]

Documentation part of message batching still does not say anything about this approach. Is it on purpose?
https://www.rabbitmq.com/tutorials/tutorial-seven-dotnet.html

[michaelklishin]

@salixzs you are welcome to contribute what you find missing. The purpose of tutorial 7 is to demonstrate publisher confirms, not be an extensive guide on publishing, though.