azure-service-bus-dotnet
azure-service-bus-dotnet copied to clipboard
[WIP] Batching
Fixes #538
The client allows to send a collection of messages (SendAsync(IList<Message>)
) which is problematic when there's a large number of messages that would not fit into the maximum message size. As a result of that, an exception is thrown when the send operation is invoked.
This PR introduces a safe batching option. A Batch
is constructed before send operation is invoked and messages are added using bool TryAdd(Message)
API. As long as batch total size doesn't exceed the maximum, messages are added to it. Otherwise, message is not added and can be carried over to the next batch. With this API messages sent will not exceed maximum message size.
What's left:
- [x] Add tests to ensure common scenarios are verified (
BatchTests
andSenderReceiverTests
) - [x] Ensure received messages are not attempted to be sent using
Batch
(logic fromMessageSender.ValidateMessage()
) - [x] Raise an issue for the next major to bring
SendAsync(Batch)
toISenderClient
--> #543 - [x] Log similar to other
SendAsync()
overloads - [x] Retrieve message max size from the broker
- [x] Pass batched messages through plugins
- ~~BLOCKED~~* Support Diagnostics similar to how
SendAsync(IList<Message>)
does it
* blocked until Diagnostics can support a different approach of registering messages. See an experimental PR comments here.
Opening this PR to discuss further implementation details
@makam this is the approach I'd like to take. If you see no issues, I'll continue with the rest of TODOs.
@vinaysurya I cannot trigger rebuilds for https://ci.appveyor.com/project/vinaysurya/azure-service-bus-dotnet. Could you please modify my permissions to allow me to do so? Thank you.
This PR could use #505 @nemakam 🙂
@makam @vinaysurya I've got everything except diagnostics. Diagnostics (except exceptions) cannot be integrated at the moment and would have to wait until an alternative option to implement it is introduced.
I'd like to start a review of this PR. The feature could be used with a caveat of not having full Diagnostics available if users require this feature until alternative diagnostics registration is available and added later.
@nemakam @vinaysurya ping
@nemakam @vinaysurya let me know when you're ready to start reviewing. Thanks.
@nemakam integrated your feedback
Back to green
Marked PR as [WIP]
until it's not and ready to be merged.
@SeanFeldman - we are moving the underlying issue to the [Azure SDK for .NET] (https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/servicebus/Microsoft.Azure.ServiceBus) repo.
Once we archive this repo, the PR will be read only and we can leverage this test case in the new repo.