[WIP] Batching #539

SeanFeldman · 2018-07-29T07:19:53Z

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:

Add tests to ensure common scenarios are verified (BatchTests and SenderReceiverTests)
Ensure received messages are not attempted to be sent using Batch (logic from MessageSender.ValidateMessage())
Raise an issue for the next major to bring SendAsync(Batch) to ISenderClient --> Define SendAsync(Batch) and CreateBatch() on ISenderClient #543
Log similar to other SendAsync() overloads
Retrieve message max size from the broker
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 · 2018-07-29T07:21:40Z

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

SeanFeldman · 2018-07-30T04:27:52Z

@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 · 2018-08-02T20:34:15Z

This PR could use #505 @nemakam 🙂

SeanFeldman · 2018-08-24T05:29:33Z

@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 · 2018-08-28T04:13:33Z

@nemakam @vinaysurya ping

SeanFeldman · 2018-09-09T03:59:09Z

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

nemakam · 2018-09-21T02:00:09Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+        /// </summary>
+        /// <param name="maximumBatchSize">Maximum batch size allowed for batch.</param>
+        /// <param name="pluginsCallback">Plugins callback to invoke on outgoing messages regisered with batch.</param>
+        public Batch(ulong maximumBatchSize, Func<Message, Task<Message>> pluginsCallback)


public Batch [](start = 8, length = 12)

I think having this constructor will cause more confusion. We already have a way to create a batch on the sender.

For testing as of now, we could make this internal.
For customers who need to test this, they could mock a Sender (in the future version) and then create a batch accordingly.

Having this constructor exposed like this will definitely create confusion to customers which is just explained in remarks (which is not the most accessible comment).

I would think especially the second param - callback is going to be even more confusing for a beginner user.
Lets make this internal for now.

nemakam · 2018-09-21T02:00:16Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Threading.Tasks;
+


nit: inside

nemakam · 2018-09-21T02:01:07Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+    using Microsoft.Azure.ServiceBus.Diagnostics;
+
+    [DebuggerDisplay("{" + nameof(DebuggerDisplay) + ",nq}")]
+    public class Batch : IDisposable


Batch [](start = 17, length = 5)

Message - MessageBatch.
Sounds more consistent. Maybe rename?

nemakam · 2018-09-21T02:01:25Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+        /// Convert batch to AMQP message.
+        /// </summary>
+        /// <returns></returns>
+        public AmqpMessage ToAmqpMessage()


public [](start = 8, length = 6)

This doesn't need to be public

nemakam · 2018-09-21T02:03:05Z

src/Microsoft.Azure.ServiceBus/Extensions/MessageDiagnosticsExtensions.cs

@@ -149,5 +150,14 @@ internal static bool TryExtractContext(this Message message, out IList<KeyValueP
            }
            return false;
        }
+
+
+        internal static void VerifyMessageIsNotPreviouslyReceived(this Message message)


VerifyMessageIsNotPreviouslyReceived [](start = 29, length = 36)

This class is dedicated to diagnostics. Lets move this out of this class

nemakam · 2018-09-21T02:10:44Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+
+            if (originalMessageData.messageId != null)
+            {
+                result.Properties.MessageId = originalMessageData.messageId;


result.Properties.MessageId = originalMessageData.messageId; [](start = 16, length = 60)

What if TryAdd succeeded, and then you tried to copy over MessageId and other properties, and you crossed your size limit?

Alternatively, the moment you receive your second TryAdd, form the result along with its updated properties so that you can validate the size.

In reply to: 219366896 [](ancestors = 219366896)

Absolutely. It also removes the need to "remember" original message system properties and tuple that stores those. Nice!

nemakam · 2018-09-21T02:15:46Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+        {
+            if (result == null)
+            {
+                throw new Exception("Batch is has been disposed and cannot be re-used.");


Exception [](start = 26, length = 9)

ObjectDisposedException

nemakam · 2018-09-21T02:16:01Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+        {
+            if (result == null)
+            {
+                throw new Exception("Batch is has been disposed and cannot be re-used.");


Batch is [](start = 37, length = 8)

typo

nemakam · 2018-09-21T02:16:57Z

src/Microsoft.Azure.ServiceBus/Core/Batch.cs

+            }
+        }
+
+        private string DebuggerDisplay => $"Batch: size={Size} message count={datas.Count} maximum size={maximumBatchSize}";


size={Size} message count= [](start = 51, length = 26)

Add a . or ; or any identifier between two properties so that its easier to read

nemakam · 2018-09-21T02:25:38Z

src/Microsoft.Azure.ServiceBus/Amqp/AmqpMessageConverter.cs

        const string PartitionIdName = "x-opt-partition-id";
-        const string ViaPartitionKeyName = "x-opt-via-partition-key";
+        internal const string PartitionKeyName = "x-opt-partition-key";
+        internal const string ViaPartitionKeyName = "x-opt-via-partition-key";


Reorder them to the beginning so that internals would come before private

SeanFeldman · 2018-10-04T22:37:32Z

@nemakam integrated your feedback

SeanFeldman · 2019-02-14T19:35:09Z

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

Adding extra info to identify why the test is failing on full framework

implemented

…ync methods

…ge size

…ded in the safe size calculation.

axisc · 2019-05-01T21:03:00Z

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

SeanFeldman requested a review from a team as a code owner July 29, 2018 07:19

SeanFeldman changed the title ~~Batching~~ [WIP] Batching Jul 29, 2018

SeanFeldman force-pushed the batching branch 3 times, most recently from 6d45105 to 78ed01d Compare July 31, 2018 21:44

SeanFeldman mentioned this pull request Aug 2, 2018

Verify Message.Size functionality #109

Closed

SeanFeldman force-pushed the batching branch from 1e06f7b to 52c1f00 Compare August 12, 2018 04:10

SeanFeldman mentioned this pull request Aug 23, 2018

[WIP] Add diagnostics to batching SeanFeldman/azure-service-bus-dotnet#1

Closed

SeanFeldman force-pushed the batching branch from 52c1f00 to 4e8673a Compare August 24, 2018 04:41

SeanFeldman changed the title ~~[WIP] Batching~~ Batching Aug 24, 2018

nemakam reviewed Sep 21, 2018

View reviewed changes

SeanFeldman force-pushed the batching branch 3 times, most recently from 5a67d61 to 323d7cd Compare October 4, 2018 08:53

SeanFeldman mentioned this pull request Jan 27, 2019

[WIP] Add support for batched receive via MessageReceiver.RegisterMessageBatchHandler #636

Closed

Einarsson pushed a commit to Einarsson/azure-service-bus-dotnet that referenced this pull request Jan 29, 2019

rename BatchMessage to MessageBatch for consistency with Azure#539

163c606

SeanFeldman self-assigned this Feb 6, 2019

SeanFeldman changed the title ~~Batching~~ [WIP] Batching Feb 14, 2019

SeanFeldman added 22 commits March 20, 2019 18:27

Initial (simple) batching implementation

4b7aecc

Updating public API

56c346e

Modify test to remove reliance on order of messages

19e50df

Adding batched send to QueueClient and TopicClient

b03695e

Remove order implied by Assert.Collection()

76d127a

Adding extra info to identify why the test is failing on full framework

Validate received messages cannot be added to Batch

369a78f

implemented

Cleanup

04f1489

Enable MessagingEventSource logging to comply with the rest of SendAs…

156dce4

…ync methods

Verify custom properties affect Batch size (serialized bytes)

51da1ae

Provide an API to create Batch initiated with supported maximum messa…

7a60434

…ge size

Pass messages in Batch through outgoing plugins

306e57e

Report exception via diagnostics

a0933fe

Add tracking TODO

3f50da5

Rename Batch to MessageBatch

a7b425d

Move extension method into appropreate class

8f98ee2

Use correct exception type

8b4e31c

Minor tweaks

0aca4b0

Ensure properties from the first message needed for a batch are inclu…

484e303

…ded in the safe size calculation.

Fix and rename batch test

c02b754

Approving API

eff85a7

Uncomment log statement

eabe617

Resolved conflict from merging v3.4.0

6e20439

SeanFeldman force-pushed the batching branch from 2cb5b71 to 6e20439 Compare March 21, 2019 00:34

brolaugh mentioned this pull request Oct 25, 2019

[FEATURE REQ] Support for safe batching with MessageSender Azure/azure-sdk-for-net#8440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Batching #539

[WIP] Batching #539

SeanFeldman commented Jul 29, 2018 •

edited

Loading

SeanFeldman commented Jul 29, 2018

SeanFeldman commented Jul 30, 2018

SeanFeldman commented Aug 2, 2018

SeanFeldman commented Aug 24, 2018

SeanFeldman commented Aug 28, 2018

SeanFeldman commented Sep 9, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

SeanFeldman Oct 4, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

nemakam Sep 21, 2018

SeanFeldman commented Oct 4, 2018

SeanFeldman commented Feb 14, 2019

axisc commented May 1, 2019

		// Licensed under the MIT license. See LICENSE file in the project root for full license information.

		using System.Threading.Tasks;

[WIP] Batching #539

Are you sure you want to change the base?

[WIP] Batching #539

Conversation

SeanFeldman commented Jul 29, 2018 • edited Loading

SeanFeldman commented Jul 29, 2018

SeanFeldman commented Jul 30, 2018

SeanFeldman commented Aug 2, 2018

SeanFeldman commented Aug 24, 2018

SeanFeldman commented Aug 28, 2018

SeanFeldman commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanFeldman commented Oct 4, 2018

SeanFeldman commented Feb 14, 2019

axisc commented May 1, 2019

SeanFeldman commented Jul 29, 2018 •

edited

Loading