-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Documentation Review for version 7 #12569
Conversation
Hello @chradek! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
@@ -49,8 +49,9 @@ export interface ServiceBusSender { | |||
* method to send. | |||
* @param options - Options bag to pass an abort signal or tracing options. | |||
* @return Promise<void> | |||
* @throws `MessagingError` with the code `MessageTooLargeError` if the provided messages do not fit in a single `ServiceBusMessageBatch`. |
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.
Shouldnt this be MessageSizeExceeded
?
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.
I actually wanted to ask about this one. Should this be a ServiceBusError
instead of a MessagingError
? Currently it is a MessagingError
with the code hardcoded as MessageTooLargeError
. I thought maybe this was because we're generating the error on the client-side instead of receiving it from the service but wasn't sure if we were making that distinction here intentionally.
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.
With the introduction of ServiceBusError
, the user should never see MessagingError
again
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.
I'll merge this PR, but please follow up with this MessagingError vs ServicebusError situation in a follow up PR
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.
Logged #12573
Co-authored-by: Harsha Nalluru <sanallur@microsoft.com>
/cc @HarshaNalluru @richardpark-msft @ramya-rao-a