Skip to content

Conversation

PIRO-V
Copy link
Member

@PIRO-V PIRO-V commented Oct 3, 2025

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@PIRO-V PIRO-V requested a review from axisc as a code owner October 3, 2025 04:26
@Copilot Copilot AI review requested due to automatic review settings October 3, 2025 04:26
@PIRO-V PIRO-V requested review from sjkwak and hmlam as code owners October 3, 2025 04:26
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables support for larger message sizes (up to 20MB) in the Azure Event Hubs Python SDK by modifying the AMQP client configuration and adding sample code to demonstrate the functionality.

  • Updates the default max message size from MAX_FRAME_SIZE_BYTES to 0 (unlimited) in the SendClient
  • Adds debugging logging to track message size configurations
  • Includes a new sample function to demonstrate sending large messages up to 17MB

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
sdk/eventhub/azure-eventhub/tests/conftest.py Comments out checkpoint store imports (likely temporary for testing)
sdk/eventhub/azure-eventhub/samples/sync_samples/send.py Adds new sample function for sending large messages and calls it in main flow
sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/link.py Adds logging configuration and debug logging for attach frames
sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/client.py Changes default max_message_size from MAX_FRAME_SIZE_BYTES to 0
sdk/eventhub/azure-eventhub/azure/eventhub/_producer_client.py Adds debug print statements to track message size calculations

import uuid
import logging

logging.basicConfig(level=logging.INFO)
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

Adding global logging configuration in a library module can interfere with application-level logging configuration. Consider removing this line or using a more targeted approach that doesn't affect the global logging state.

Suggested change
logging.basicConfig(level=logging.INFO)

Copilot uses AI. Check for mistakes.

Comment on lines 362 to 363
print(f">>> _get_max_message_size: _max_message_size_on_link = {self._max_message_size_on_link}")
print(f">>> _get_max_message_size: MAX_MESSAGE_LENGTH_BYTES = {self._amqp_transport.MAX_MESSAGE_LENGTH_BYTES}")
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

Debug print statements should be removed from production code. Consider using proper logging with appropriate log levels instead of print statements.

Suggested change
print(f">>> _get_max_message_size: _max_message_size_on_link = {self._max_message_size_on_link}")
print(f">>> _get_max_message_size: MAX_MESSAGE_LENGTH_BYTES = {self._amqp_transport.MAX_MESSAGE_LENGTH_BYTES}")
_LOGGER.debug(">>> _get_max_message_size: _max_message_size_on_link = %s", self._max_message_size_on_link)
_LOGGER.debug(">>> _get_max_message_size: MAX_MESSAGE_LENGTH_BYTES = %s", self._amqp_transport.MAX_MESSAGE_LENGTH_BYTES)

Copilot uses AI. Check for mistakes.

Comment on lines 803 to 804
print(f">>> create_batch: max_size_in_bytes parameter = {max_size_in_bytes}")
print(f">>> create_batch: _max_message_size_on_link = {self._max_message_size_on_link}")
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

Debug print statements should be removed from production code. Consider using proper logging with appropriate log levels instead of print statements.

Suggested change
print(f">>> create_batch: max_size_in_bytes parameter = {max_size_in_bytes}")
print(f">>> create_batch: _max_message_size_on_link = {self._max_message_size_on_link}")
logger.debug(f">>> create_batch: max_size_in_bytes parameter = {max_size_in_bytes}")
logger.debug(f">>> create_batch: _max_message_size_on_link = {self._max_message_size_on_link}")

Copilot uses AI. Check for mistakes.

Try to send one single oversized message to Event Hub.
Now with explicit large max_size_in_bytes to allow 20MB messages.
"""
payload = "X" * (size_mb * 1024 * 1024) # 20 MB string
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

The comment says '20 MB string' but the parameter is size_mb which defaults to 17MB. The comment should reflect the actual size being created or be more generic.

Suggested change
payload = "X" * (size_mb * 1024 * 1024) # 20 MB string
payload = "X" * (size_mb * 1024 * 1024) # Create a string of size_mb megabytes

Copilot uses AI. Check for mistakes.

payload = "X" * (size_mb * 1024 * 1024) # 20 MB string
try:
# Explicitly set max_size_in_bytes to allow large messages
batch = producer.create_batch(max_size_in_bytes=19 * 1024 * 1024) # 20MB limit
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

The comment says '20MB limit' but the calculation is 19 * 1024 * 1024 which equals 19MB, not 20MB. The comment should accurately reflect the actual limit being set.

Suggested change
batch = producer.create_batch(max_size_in_bytes=19 * 1024 * 1024) # 20MB limit
batch = producer.create_batch(max_size_in_bytes=19 * 1024 * 1024) # 19MB limit (1MB below 20MB service max for safety)

Copilot uses AI. Check for mistakes.

@PIRO-V
Copy link
Member Author

PIRO-V commented Oct 3, 2025

@microsoft-github-policy-service agree

@PIRO-V
Copy link
Member Author

PIRO-V commented Oct 3, 2025

@microsoft-github-policy-service agree company="Microsoft"

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.

1 participant