Skip to content
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

[FEATURE REQ] Support for safe batching with MessageSender #8440

Closed
brolaugh opened this issue Oct 25, 2019 · 18 comments
Closed

[FEATURE REQ] Support for safe batching with MessageSender #8440

brolaugh opened this issue Oct 25, 2019 · 18 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. Service Attention Workflow: This issue is responsible by Azure service team. Service Bus
Milestone

Comments

@brolaugh
Copy link

Is your feature request related to a problem? Please describe.
As described in Azure/azure-service-bus-dotnet#539

The client allows to send a collection of messages (SendAsync(IList)) 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.

Describe the solution you'd like
I'd like to write the following code without worrying about an MessageSizeExceededException

await messageSender.SendAsync(bigListOfMessages);

But since there already a SendAsync method that takes IList<Message> one could name it SendChunkedAsync(), SendBatchAsync() or something else that doesn't conflict.

I'm thinking that this method would use the current SendAsync(IList<Message>) method to send it's chunks of Messages in order to pack them into as few AMPQ messages as possible.

Describe alternatives you've considered

  1. I've tried to chunk the messages my self as described in this article but given that the Size property of the Message class only accounts for body size.

  2. This is the my current solution from my understanding sends all the messages individually. Or at least it appears so on the when looking at the code.

var sendTasks = messages
    .Select(m => messageSender.SendAsync(m))
    .ToArray();
await Task.WaitAll(sendTasks);
@triage-new-issues triage-new-issues bot added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Oct 25, 2019
@AlexGhiondea AlexGhiondea added Client This issue points to a problem in the data-plane of the library. Service Attention Workflow: This issue is responsible by Azure service team. Service Bus labels Oct 26, 2019
@ghost
Copy link

ghost commented Oct 26, 2019

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @jfggdl

@triage-new-issues triage-new-issues bot removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Oct 26, 2019
@ghost
Copy link

ghost commented Oct 26, 2019

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @jfggdl

@jfggdl jfggdl added this to the Sprint 164 milestone Oct 28, 2019
@jfggdl
Copy link

jfggdl commented Oct 28, 2019

@brolaugh, thanks for creating this issue. Our engineering team is investigating into different approaches to make your coding simpler.

@nemakam nemakam added the feature-request This issue requires a new behavior in the product in order be resolved. label Dec 11, 2019
@ericsampson
Copy link

@axisc since @SeanFeldman had a WIP PR in the old repo, would it be possible to resurrect that code to get this feature request moving forward? It's really unfortunate that there's no way to do safe batching with the SDK. Thanks!
https://github.com/Azure/azure-service-bus-dotnet/pull/539/files

@SeanFeldman
Copy link
Contributor

The old repository was moved into this mono-repo. New feature development is on hold, that's why I don't try to continue with the PR. Sorry, @ericsampson.

@brolaugh
Copy link
Author

@SeanFeldman but given that this feature request is due on 17th of January which is rather soon, it wouldn't been unreasonable for them to continue on that path you started in order to reduce workload. Right?

@SeanFeldman
Copy link
Contributor

@brolaugh I'm not a Microsoft employee and don't know what's the milestone planning is. Perhaps it's still planned and worked on internally? No idea, sorry.

@AlexGhiondea would you be able to help with a clarification? Thank you.

@AlexGhiondea
Copy link
Contributor

@brolaugh this issue is about our released library for Service Bus. @binzywu would know more about this.

@brolaugh
Copy link
Author

@axisc The milestone that this issue has been attached to is as of now over due by 12 days so I would have expected this feature to be implemented by now. Maybe you have structured your milestones in some other ways. Please clarify what the ETA of this feature is or how your task management is structured.

@AlexGhiondea
Copy link
Contributor

@brolaugh as this is about our shipped Service Bus library @jfggdl can help find the right person to look at this.

@brolaugh
Copy link
Author

brolaugh commented Feb 1, 2020

Okay I'll wait for a response from @jfggdl then :)

@wolszakp
Copy link

Hi guys,
This feature I will accept with open hands :)
I googled and see that in old repo there was a lot of work done to have safe batching mechanism.
Is there any news about it?

@brolaugh
Copy link
Author

Hi guys,
This feature I will accept with open hands :)
I googled and see that in old repo there was a lot of work done to have safe batching mechanism.
Is there any news about it?

@SeanFeldman was the one working on it and he said the following earlier in this thread.

The old repository was moved into this mono-repo. New feature development is on hold, that's why I don't try to continue with the PR. Sorry, @ericsampson.

@jsquire
Copy link
Member

jsquire commented Feb 26, 2020

Moving this to the backlog milestone, as the associated sprint has passed.

@axisc: I removed the outdated assignment. Would you be so kind as to help route to the appropriate individual?

@azure-sdk azure-sdk added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Sep 24, 2020
@ColeSiegelTR
Copy link

Hi @axisc , @jsquire

Is there any chance of having this feature request implemented? I've seen various PRs and issues raised related to this since early 2017 but it seems like any progress may have come to a standstill.

@brolaugh did you find a viable work-around in the mean time? Would you mind sharing if so? Thanks!

@brolaugh
Copy link
Author

@ColeSiegelTR I simply sent all messages one by one like described in my original post.

@jsquire
Copy link
Member

jsquire commented Dec 22, 2020

Have you considered using Azure.Messaging.ServiceBus, which recently went GA and is now considered the current generation library for Service Bus? It offers the ability to build a batch that is aware of size limits and that can be sent without the potential of exceeding the limit and triggering an exception.

For example:

// add the messages that we plan to send to a local queue
Queue<ServiceBusMessage> messages = new Queue<ServiceBusMessage>();
messages.Enqueue(new ServiceBusMessage("First message"));
messages.Enqueue(new ServiceBusMessage("Second message"));
messages.Enqueue(new ServiceBusMessage("Third message"));

// create a message batch that we can send
// total number of messages to be sent to the Service Bus queue
int messageCount = messages.Count;

// while all messages are not sent to the Service Bus queue
while (messages.Count > 0)
{
    // start a new batch
    using ServiceBusMessageBatch messageBatch = await sender.CreateMessageBatchAsync();

    // add the first message to the batch
    if (messageBatch.TryAddMessage(messages.Peek()))
    {
        // dequeue the message from the .NET queue once the message is added to the batch
        messages.Dequeue();
    }
    else
    {
        // if the first message can't fit, then it is too large for the batch
        throw new Exception($"Message {messageCount - messages.Count} is too large and cannot be sent.");
    }

    // add as many messages as possible to the current batch
    while (messages.Count > 0 && messageBatch.TryAddMessage(messages.Peek()))
    {
        // dequeue the message from the .NET queue as it has been added to the batch
        messages.Dequeue();
    }

    // now, send the batch
    await sender.SendMessagesAsync(messageBatch);

    // if there are any remaining messages in the .NET queue, the while loop repeats
}

More information on Azure.Messaging.ServiceBus can be found on its README and samples.

//cc: @JoshLove-msft

@ramya-rao-a
Copy link
Contributor

Hello all,

The newer package Azure.Messaging.ServiceBus is available as of November 2020. While the older package Microsoft.Azure.ServiceBus will continue to receive critical bug fixes, we strongly encourage you to upgrade. Read the migration guide for more details.

Closing this issue as there are no plans to add the feature discussed here to the older Microsoft.Azure.ServiceBus package and @jsquire has provided pointers on how to use this feature in the Azure.Messaging.ServiceBus

Thanks for your patience!

@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. Service Attention Workflow: This issue is responsible by Azure service team. Service Bus
Projects
None yet
Development

No branches or pull requests