-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce IBasicPublishBatch #368
Conversation
Batch publish allows sending multiple messages in one stream on the socket. Sending in batches improves performance by reducing the number of TCP/IP messages sent and TCP/IP acknowledgments received. This change compiles the commands for all publish messages into a memory buffer that is posted to the socket as a single stream. Closing the socket/model/connection before the buffer has completed sending does not guarantee message delivery.
Batch Publish
Addition of API items for BatchPublish
de86693
to
e8070ec
Compare
{ | ||
public interface IMessageBatch | ||
{ | ||
void Add(string exchange, string routingKey, bool mandatory, IBasicProperties properties, byte[] body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love an overload called AddRange that took an IEnumerable of Message that contained these elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public void Publish() | ||
{ | ||
model.SendCommands(commands); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the intention is that you'll be creating a new instance via CreateBasicPublishBatch()
and not reusing these because the constructor is internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
@bording Thanks for reviewing. |
@kjnilsson 👍 For the API changes here also. |
@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 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. |
Maybe you could grab the |
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. |
@michaelklishin @lukebakken I think we're there with this PR unless either of you would like any changes? |
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 👍 |
} | ||
|
||
public void WriteFrameSet(IList<OutboundFrame> f) | ||
public void WriteFrameSet(List<OutboundFrame> f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is better it is also a breaking change, was this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was but given how often IFrameHandler appears in (unnecessarily) public APIs we should probably revert it. :/
@@ -77,6 +77,6 @@ public interface IFrameHandler | |||
|
|||
void WriteFrame(OutboundFrame frame); | |||
|
|||
void WriteFrameSet(IList<OutboundFrame> frames); | |||
void WriteFrameSet(List<OutboundFrame> frames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is better it is also a breaking change, was this intentional?
BasicPublishBatch can be used for more efficient batch publishing that will better utilise TCP and provides more throughput. [#152041636]
3d8c7f3
to
74c9c24
Compare
There is a pre-release available now that contain this feature: https://www.nuget.org/packages/RabbitMQ.Client/5.1.0-pre2 |
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 |
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. |
Documentation part of message batching still does not say anything about this approach. Is it on purpose? |
@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. |
I've changed @YulerB 's
BatchMessage
for anIBasicPublishBatch
approach with anAdd
method with a similar signature toIModel.BasicPublish
. This allows messages for arbitrary exchanges and routing keys to be grouped together in a singlewrite(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: