fix(servicebus): Read max-message-batch-size vendor property for batch sizing#46197
Open
EldertGrootenboer wants to merge 12 commits intoAzure:mainfrom
Open
fix(servicebus): Read max-message-batch-size vendor property for batch sizing#46197EldertGrootenboer wants to merge 12 commits intoAzure:mainfrom
EldertGrootenboer wants to merge 12 commits intoAzure:mainfrom
Conversation
…lient (Azure#44999) - Pass **kwargs from __init__ to _build_pipeline() in both sync and async ServiceBusAdministrationClient so transport kwargs (connection_verify, transport, policies, ssl_context) reach the transport layer - Add unit tests verifying kwargs forwarding for both sync and async clients - Matches pattern from Azure#26015 fix for ServiceBusClient and azure-core base
…h sizing - Read com.microsoft:max-message-batch-size from AMQP sender link remote_properties to correctly limit batch size on Premium large-message entities where max-message-size can be up to 100 MB but the batch limit is 1 MB - Add get_remote_max_message_batch_size() to both pyamqp and uamqp transport layers (uamqp returns None as it doesn't expose link properties, falling back to existing tier-based inference) - Update both sync and async senders to prefer vendor property - Add comprehensive unit tests for transport method and sender batch size inference
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Azure Service Bus sender batch sizing to prefer the service-reported AMQP vendor link property com.microsoft:max-message-batch-size, with a fallback to the existing tier-based inference when the property isn’t available (notably for the uAMQP transport). It also adjusts the management client to forward **kwargs into pipeline construction and adds regression tests.
Changes:
- Read
com.microsoft:max-message-batch-sizefrom pyamqp linkremote_propertiesand use it to set_max_batch_size_on_link(sync + async senders). - Add a uAMQP transport stub for the same API (returns
None, triggering fallback behavior). - Add unit tests for batch sizing and for forwarding management client
**kwargsinto_build_pipeline.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/servicebus/azure-servicebus/azure/servicebus/_transport/_pyamqp_transport.py | Adds get_remote_max_message_batch_size() reading from pyamqp link remote_properties. |
| sdk/servicebus/azure-servicebus/azure/servicebus/_transport/_uamqp_transport.py | Adds get_remote_max_message_batch_size() stub returning None for uAMQP. |
| sdk/servicebus/azure-servicebus/azure/servicebus/_servicebus_sender.py | Uses vendor batch-size property when available, otherwise falls back to tier inference. |
| sdk/servicebus/azure-servicebus/azure/servicebus/aio/_servicebus_sender_async.py | Async sender mirrors the same vendor-property-first batch sizing behavior. |
| sdk/servicebus/azure-servicebus/azure/servicebus/management/_management_client.py | Forwards **kwargs into _build_pipeline(**kwargs). |
| sdk/servicebus/azure-servicebus/azure/servicebus/aio/management/_management_client_async.py | Async management client mirrors _build_pipeline(**kwargs) forwarding. |
| sdk/servicebus/azure-servicebus/tests/test_batch_sizing.py | Adds unit tests covering vendor property presence/absence and tier fallback logic. |
| sdk/servicebus/azure-servicebus/tests/test_mgmt_client_kwargs.py | Adds regression tests ensuring management client **kwargs reach _build_pipeline (sync + async). |
- Check both bytes and str forms of vendor property key since pyamqp decodes AMQP symbols as bytes (b'com.microsoft:max-message-batch-size') - Update tests to use bytes keys matching production behavior - Add test for str key fallback - Remove unused imports, fix docstring
- Add get_remote_max_message_batch_size to AmqpTransport base class for type safety (mypy/IDE) - Use MAX_MESSAGE_LENGTH_BYTES constant in tests instead of hardcoded 256*1024
- Add pylint disable=unused-argument on uamqp stub method - Add async sender batch size inference tests for parity with sync tests
The Build Analyze CI job runs mypy on every package and treats it as release-blocking. azure-servicebus had 58 latent mypy errors on main that surfaced once mypy became enforced. Changes: - _pyamqp_transport.py / _uamqp_transport.py: in to_outgoing_amqp_message, bind cast() results to local variables (`header`, `props`) instead of reassigning class attributes. mypy does not narrow Optional types through reassignment of class attributes; it does narrow through fresh local bindings. This removes 45 union-attr errors with no behavior change. - _uamqp_transport.py: add `# type: ignore[call-overload]` for the single bytes-keyed dict.get on properties (mypy can't infer the key type from the SESSION_LOCKED_UNTIL constant). - _common/message.py: add `# type: ignore[assignment]` on 12 constructor lines where Optional parameters are assigned to non-Optional property setters. Fixing the setters properly is a separate cleanup that should not ride along with this batch-size bug fix.
…tional Address Copilot review feedback on PR Azure#46197: rather than suppressing the 12 [assignment] mypy errors with `# type: ignore`, widen the setter signatures to match the actual runtime behavior. All 12 setters already accept `None` at runtime - they assign to the underlying Optional AMQP property. The previous non-Optional signatures were too narrow, which is why the constructor (which legitimately passes Optional values) needed suppressions. Changes: - session_id, message_id, content_type, correlation_id, subject, reply_to, reply_to_session_id, to, partition_key: `str` -> `Optional[str]` - application_properties: `Dict[...]` -> `Optional[Dict[...]]` - time_to_live: `Union[timedelta, int]` -> `Optional[Union[timedelta, int]]` - scheduled_enqueue_time_utc: `datetime` -> `Optional[datetime]` - Remove all 12 `# type: ignore[assignment]` comments from constructor This is a backward-compatible widening (Liskov-safe). mypy still passes with zero errors.
EldertGrootenboer
added a commit
to Azure/azure-sdk-for-js
that referenced
this pull request
May 8, 2026
…ch sizing (#38049) ## Problem On Premium large-message Service Bus entities, the AMQP link's `max-message-size` can be up to 100 MB, but the broker enforces a 1 MB limit for batch sends. The JS SDK reads `max-message-size` (via `this.link.maxMessageSize`) for batch sizing, so `createMessageBatch()` accepts batches up to 100 MB that the broker then rejects. Related: [azure-service-bus#708](Azure/azure-service-bus#708) ## Fix Read the `com.microsoft:max-message-batch-size` vendor property from the AMQP sender link attach frame, which correctly reports 256 KB (Standard) / 1 MB (Premium) independent of entity-level `max-message-size`. When the vendor property is absent (older service versions), fall back to `Math.min(maxMessageSize, defaultMaxBatchSize)` where `defaultMaxBatchSize` is 1 MB — this caps the batch size to prevent using the raw `max-message-size` (which can be up to 100 MB on Premium large-message entities). ### Changes - **`messageSender.ts`**: Add `defaultMaxBatchSize` constant (1 MB); add `getMaxBatchSizeFromLink()` that reads the vendor property from `this.link.properties`, falling back to `Math.min(maxMessageSize, defaultMaxBatchSize)`; update `createBatch()` to use the new method - **`messageSender.spec.ts`**: Unit tests for vendor property present, absent (with 1 MB cap), undefined properties, wrong type, zero value, user override, rejection of oversized `maxSizeInBytes`, Standard tier ### Cross-SDK alignment | SDK | Status | |-----|--------| | .NET | [PR #57941](Azure/azure-sdk-for-net#57941) | | Java | [PR #48214](Azure/azure-sdk-for-java#48214) | | Go | [PR #26530](Azure/azure-sdk-for-go#26530) | | Python | [PR #46197](Azure/azure-sdk-for-python#46197) | --------- Co-authored-by: Eldert Grootenboer (from Dev Box) <egrootenboer@microsoft.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On Premium large-message Service Bus entities, the AMQP link's
max-message-sizecan be up to 100 MB, but the broker enforces a 1 MB limit for batch sends. The Python SDK infers batch size from the tier by comparingmax-message-sizeagainst known thresholds ΓÇö this works today but is fragile and doesn't use the actual service-reported value.Related: azure-service-bus#708
Fix
Read the
com.microsoft:max-message-batch-sizevendor property from the AMQP sender linkremote_properties, which correctly reports 256 KB (Standard) / 1 MB (Premium) independent of entity-levelmax-message-size. Fall back to the existing tier-based inference when the vendor property is absent (including the uAMQP transport which doesn't expose link properties).Changes
_pyamqp_transport.py: Addget_remote_max_message_batch_size()reading fromhandler._link.remote_properties_uamqp_transport.py: Add stubget_remote_max_message_batch_size()returningNone(uAMQP transport doesn't expose link properties; falls back to tier-based inference)_servicebus_sender.py+_servicebus_sender_async.py: Prefer vendor property, fall back to tier-based inference for both sync and async senderstest_batch_sizing.py: 12 new unit tests: transport method (8 tests: present, absent, wrong type, zero, negative, Standard, missing attr) + sender inference (4 tests: vendor override, Premium fallback, Standard fallback, Standard with vendor)Cross-SDK alignment
CI unblock (added 2026-05-08)
Two Build Analyze sub-tasks were failing on this PR:
_version.pywas still7.14.3; the script then matched the dated 7.14.3 entry and failed because it expected the latest entry to be undated. Fixed by bumping_version.pyto 7.14.4 (commit9027a596eb).mainsurfaced once mypy became release-blocking. Fixed by bindingcast()results to local variables in_pyamqp_transport.py/_uamqp_transport.py(45 errors removed cleanly), one# type: ignore[call-overload]for a bytes-keyeddict.get, and 12# type: ignore[assignment]on constructor lines in_common/message.pywhere Optional parameters are passed to non-Optional setters. The setter signature cleanup is intentionally left for a follow-up so this PR stays focused on the batch-size fix (commit0b35cd33cf).Verified locally:
mypy azurereportsSuccess: no issues found in 112 source files; new + existing message/AMQP unit tests still pass.