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

[ServiceBus] add message batch size limit #33300

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeremymeng
Copy link
Member

Even though Service Bus Premium namespaces support large messages up to 100 MB, batching of large messages is
not supported.

https://learn.microsoft.com/en-us/azure/service-bus-messaging/service-bus-premium-messaging#large-messages-support

Currently we use the max message size as the max allowed batch size. This allows customers to add large
messages into the batch only to get a cryptic "link **** is force detached" error from the service after
sending the batch, as reported in

#33278

This PR updates createBatch to use two pre-defined maximum batch sizes, for Standard and Premium
respectively, as the batch limits when creating batches, thus enforcing the proper limits on messages being
added into the batch.


Packages impacted by this PR

@azure/service-bus

Related PR

Azure/azure-sdk-for-python#38313

Even though Service Bus Premium namespaces support large messages up to 100 MB, batching of large messages is
not supported.

https://learn.microsoft.com/en-us/azure/service-bus-messaging/service-bus-premium-messaging#large-messages-support

Currently we use the max message size as the max allowed batch size. This allows customers to add large
messages into the batch only to get a cryptic "link **** is force detached" error from the service after
sending the batch, as reported in

Azure#33278

This PR updates `createBatch` to use two pre-defined maximum batch sizes, for Standard and Premium
respectively, as the batch limits when creating batches, thus enforcing the proper limits on messages being
added into the batch.
@jeremymeng
Copy link
Member Author

/azp run js - service-bus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a fan of client-side validation but it seems like this train has already left a long time ago. Is there a way to dynamically retrieve these batch size limits from the service?

Comment on lines +406 to +410
if (maxMessageSize > maxBatchSizePremium) {
maxMessageSize = maxBatchSizePremium;
} else {
maxMessageSize = maxBatchSizeStandard;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we speculating here whether we're connected to a premium resource? Could this be done in a more robust way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maxMessageSize is from the service. It is a good indicator since for standard it's 256 KB and for premium it is at least 1 MB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the service introduces a new tier with a different batch size, would this code still work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will need to update the code if that happens. Currently I am not aware of any way to retrieve the batch limit from the service. Let me ask the service team.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We hope that existing libraries can continue to work. Does the service API use API versions to guard behavioral changes? If not, this code could be easily broken by a future change.

@jeremymeng
Copy link
Member Author

I am not a fan of client-side validation but it seems like this train has already left a long time ago.

In general, yes we should avoid doing validation on client side. In this case the service is giving an error that is not helping customer. A better error would have been ideal. We can help customers here.

Is there a way to dynamically retrieve these batch size limits from the service?

The batch size limits are constants according to the doc

image

@deyaaeldeen
Copy link
Member

The batch size limits are constants according to the doc

Are these guaranteed to never change in the future?

@jeremymeng
Copy link
Member Author

Are these guaranteed to never change in the future?

Great question! I will ask the service team and get back to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants