-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Service Bus] Caching AMQP messages within AmqpMessageBatch #30305
Conversation
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageBatch.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Outdated
Show resolved
Hide resolved
API change check API changes are not detected in this pull request. |
/azp run net - servicebus- tests |
No pipelines are associated with this pull request. |
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpClient.cs
Outdated
Show resolved
Hide resolved
/azp run net - servicebus - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpClient.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageBatch.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageBatch.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Core/TransportMessageBatch.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: JoshLove-msft <[email protected]>
…ssageBatch.cs Co-authored-by: Jesse Squire <[email protected]>
This pull request is protected by Check Enforcer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; I'll defer to Josh for the authoritative approval.
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusModelFactory.cs
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Sender/ServiceBusMessageBatch.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Sender/ServiceBusSender.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Sender/ServiceBusSender.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpMessageConverter.cs
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Diagnostics/EntityScopeFactory.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/src/Sender/ServiceBusSender.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a few remaining comments.
/azp run net - servicebus - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
…sSender.cs Co-authored-by: JoshLove-msft <[email protected]>
…tyScopeFactory.cs Co-authored-by: JoshLove-msft <[email protected]>
/azp run net - servicebus - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
) * switching to amqp batch * changing readonly type * updating model factory * making AmqpMessageConverter mockable * big test revamp * restructuring AmqpMessageBatchTests * fixing reserved size overhead * additional tests * disposing amqp message * Update sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpClient.cs Co-authored-by: JoshLove-msft <[email protected]> * Update sdk/servicebus/Azure.Messaging.ServiceBus/src/Core/TransportMessageBatch.cs Co-authored-by: Jesse Squire <[email protected]> * indent * additional tests * removing servicebusmessages from batch pt 1 * remove sb messages from batch pt 2 * test updates * additional test updates * additional formatting and tests * last test fix * build fix * fixing size calculation * small test update * Update sdk/servicebus/Azure.Messaging.ServiceBus/tests/Amqp/AmqpSenderTests.cs Co-authored-by: JoshLove-msft <[email protected]> * Update sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusModelFactory.cs Co-authored-by: JoshLove-msft <[email protected]> * Update sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusModelFactory.cs Co-authored-by: JoshLove-msft <[email protected]> * fixing field names * test updates * removing diagnostics update * test updates * feedback updates + test updateS * static property * name change to default * moving rule manager methods * remove duplicate instrumentation * removing unnecessary code * test fix * removing unnecessary code * Update sdk/servicebus/Azure.Messaging.ServiceBus/src/Sender/ServiceBusSender.cs Co-authored-by: JoshLove-msft <[email protected]> * Update sdk/servicebus/Azure.Messaging.ServiceBus/src/Diagnostics/EntityScopeFactory.cs Co-authored-by: JoshLove-msft <[email protected]> * small updates * small change * Switching method to private Co-authored-by: JoshLove-msft <[email protected]> Co-authored-by: Jesse Squire <[email protected]>
The main focus of the changes here is to implement the changes in
AmqpMessageBatch
in Service Bus that have already been made to theEventDataBatch
in Event Hubs. Rather than serializing eachServiceBusMessage
twice before send (once to measure size, and once to actually send), the improvedAmqpMessageBatch
simply serializes them and clones theAmqpMessage
instance, allowing for a more efficient batching experience.See #28866
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.