azure-service-bus-dotnet icon indicating copy to clipboard operation
azure-service-bus-dotnet copied to clipboard

[WIP] Batching

Open SeanFeldman opened this issue 5 years ago • 10 comments

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 and SenderReceiverTests)
  • [x] Ensure received messages are not attempted to be sent using Batch (logic from MessageSender.ValidateMessage())
  • [x] Raise an issue for the next major to bring SendAsync(Batch) to ISenderClient --> #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

SeanFeldman avatar Jul 29 '18 07:07 SeanFeldman

@makam this is the approach I'd like to take. If you see no issues, I'll continue with the rest of TODOs.

SeanFeldman avatar Jul 29 '18 07:07 SeanFeldman

@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.

SeanFeldman avatar Jul 30 '18 04:07 SeanFeldman

This PR could use #505 @nemakam 🙂

SeanFeldman avatar Aug 02 '18 20:08 SeanFeldman

@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.

SeanFeldman avatar Aug 24 '18 05:08 SeanFeldman

@nemakam @vinaysurya ping

SeanFeldman avatar Aug 28 '18 04:08 SeanFeldman

@nemakam @vinaysurya let me know when you're ready to start reviewing. Thanks.

SeanFeldman avatar Sep 09 '18 03:09 SeanFeldman

@nemakam integrated your feedback

SeanFeldman avatar Oct 04 '18 22:10 SeanFeldman

Back to green

SeanFeldman avatar Oct 05 '18 04:10 SeanFeldman

Marked PR as [WIP] until it's not and ready to be merged.

SeanFeldman avatar Feb 14 '19 19:02 SeanFeldman

@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.

axisc avatar May 01 '19 21:05 axisc