Azure / azure-service-bus-dotnet

☁️ .NET Standard client library for Azure Service Bus
https://azure.microsoft.com/services/service-bus
Other
235 stars 120 forks source link

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

Closed Einarsson closed 5 years ago

Einarsson commented 5 years ago

add registerMessageHandler on receiver clients that supports batching

This is a proposed change to add batch support to the receiver clients. RegisterMessageHandler currently only processes one message at a time. Batching it would make it easier to handle chunks of messages if you need it to optimize performance towards data sources like SQL etc.

https://github.com/Azure/azure-service-bus-dotnet/issues/582

Changes aren't 100% complete. I have only run manual tests and thus haven't written any unit tests yet. I would like to know if it is at all a viable implementation before proceeding. There might also be formatting issues and other details that might be improved and then I'll happily take the feedback and fix it when I get around to it.

TL;DR; If you like this approach, I can continue working on it with your feedback; else throw it. :)

msftclas commented 5 years ago

CLA assistant check
All CLA requirements met.

Einarsson commented 5 years ago

Thanks for inputs @SeanFeldman!

One thing though; I havn't figured out a way to nest generics in XML comments; Take this one for example:

/// Use <see cref="RegisterMessageHandler(Func{Message,CancellationToken,Task}, MessageHandlerOptions)"/> to configure the settings of the pump.</remarks>

It says incorrect syntax if you do RegisterMessageHandler(Func{List{Message},CancellationToken,Task}

I'll look into the changes you've proposed. :) Should also be possible to avoid an unnecessary breaking change.

SeanFeldman commented 5 years ago

It says incorrect syntax if you do RegisterMessageHandler(Func{List{Message},CancellationToken,Task}

What you're doing should work. Are you on the latest of VS2017?

Should also be possible to avoid an unnecessary breaking change.

Indeed.

Einarsson commented 5 years ago

I'll check the commenting again, I updated VS like last month but sometimes VS do weird stuff for no apparent reason. :)

Thanks for your time! Very good inputs!

Einarsson commented 5 years ago

Interesting.

This works: /// Use <see cref="RegisterBatchMessageHandler(Func{IList{Message},CancellationToken,Task}, BatchMessageHandlerOptions)"/> to configure the settings of the pump.</remarks>

This does not: /// This handler(<see cref="Func{IList{Message}, CancellationToken, Task}"/>>) is awaited on every time a new message is received by the receiver.

It gives: CS1584 C# XML comment has syntactically incorrect cref attribute CS1658 C# Type parameter declaration must be an identifier not a type. See also error CS0081.

Appearently a known problem: https://stackoverflow.com/questions/684982/referring-to-a-generic-type-of-a-generic-type-in-c-sharp-xml-documentation/797364

Einarsson commented 5 years ago

Sorted out lot of proposed changes.

However issue with the XML comments still persists and are unsolvable, not sure how to proceed with that. It could work if you introduce some MessageBatch object instead of IList but the idea of XML comments dictating code structure sickens me. Might be viable if you'd want some kind of metadata regarding the messages included in the batch, otherwise its just unnecessary clutter.

SeanFeldman commented 5 years ago

The build is OK with XMLDocs, but it's failing on tests. Could you please verify those?

Einarsson commented 5 years ago

Ran the testa successfully yesterday. I can double check later.

The build is ok with XML comments because i havnt changed to List yet. Seems to be no real way around it.

Einarsson commented 5 years ago

Tests runnin. :)

SeanFeldman commented 5 years ago

@Einarsson great! Could you add a test to verify this is working as expected?

Also, I'm sure @nemakam 's review will be asking for setting up a longer running test app to see if this can run for longer period of time w/o any issues (a sender that sends messages and a receiver that receives batches).

Einarsson commented 5 years ago

Will do @SeanFeldman ! Gonna research the existing tests to setup a test in a similar fashion. :)

SeanFeldman commented 5 years ago

You will need an environment variable with connection string to the namespace with a set of predefined entities. All this is documented here.

Einarsson commented 5 years ago

I've added some tests that uses the subscription client and messageBatchHandler. I've tested it manually with console apps as well before where I sent 10 000 or more messages in batches of max 100.

SeanFeldman commented 5 years ago

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

axisc commented 5 years ago

@Einarsson we're closing this PR given that this repo has been migrated to Azure SDK for .NET.

If you feel that you still want to pursue this feature, please open an issue in the new repo and we can discuss this further.