Skip to content

Expose more types for C++; reorganized some public types a bit #2608

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

Merged
merged 2 commits into from
May 16, 2025

Conversation

LarryOsterman
Copy link
Member

This PR adds a few public types which are needed for the C++ AMQP implementation.

It also moves some types from the global namespace and into a builder and message namespace.

Verified that the C++ codebase builds and works with all of these changes.

Bonus: Fixed two late breaking suggestions for the EventHubs performance tests.

@Copilot Copilot AI review requested due to automatic review settings May 14, 2025 23:58
@github-actions github-actions bot added the Azure.Core The azure_core crate label May 14, 2025
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 refactors and exposes more public types needed for the C++ AMQP implementation and reorganizes code into clearer namespaces. It also updates some benchmarks for EventHubs and introduces minor API improvements.

  • Refactors public types by moving some globals into the builder and message namespaces.
  • Updates benchmark configurations with a fallback for TestMode and adjusts measurement times.
  • Introduces a mutable accessor in AmqpComposite and reorganizes module exports.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
sdk/eventhubs/azure_messaging_eventhubs/tests/eventhubs_round_trip.rs Updated import for AmqpMessageProperties to use the new message namespace.
sdk/eventhubs/azure_messaging_eventhubs/tests/eventhubs_producer.rs Adjusted import for AmqpMessageProperties to match new structure.
sdk/eventhubs/azure_messaging_eventhubs/src/models/mod.rs Reorganized import paths for AmqpMessageId.
sdk/eventhubs/azure_messaging_eventhubs/src/models/event_data.rs Updated import for AmqpMessageProperties to follow new namespacing.
sdk/eventhubs/azure_messaging_eventhubs/src/consumer/mod.rs Adjusted and reordered imports for clarity.
sdk/eventhubs/azure_messaging_eventhubs/benches/benchmarks.rs Modified benchmark setup with TestMode fallback and changed measurement times.
sdk/core/azure_core_amqp/src/value.rs Added a new mutable getter (value_mut) for AmqpList.
sdk/core/azure_core_amqp/src/messaging.rs Changed the builders module to pub(crate) to clarify its accessibility.
sdk/core/azure_core_amqp/src/lib.rs Reorganized module exports and introduced public builder and message submodules.
sdk/core/azure_core_amqp/src/fe2o3/messaging/mod.rs Updated import to include AmqpMessageProperties.
sdk/core/azure_core_amqp/src/fe2o3/error.rs Simplified error conversion using AmqpSymbol directly.
sdk/core/azure_core_amqp/src/error.rs Removed an unused, commented sender error export.
Comments suppressed due to low confidence (1)

sdk/eventhubs/azure_messaging_eventhubs/benches/benchmarks.rs:145

  • The benchmark measurement time was increased drastically from 205 to 2200 seconds; please confirm if this extended duration is intentional, as it may significantly affect benchmark execution times.
.measurement_time(std::time::Duration::new(2200, 0));

Copy link

github-actions bot commented May 15, 2025

API Change Check

APIView identified API level changes in this PR and created the following API reviews

azure_core_amqp
azure_messaging_eventhubs

@LarryOsterman LarryOsterman requested review from Copilot and heaths May 15, 2025 17:57
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 exposes additional public types for the C++ AMQP implementation and reorganizes some public types into the builder and message namespaces. It also fixes issues related to the EventHubs performance tests by updating benchmark configurations and improving import paths.

Reviewed Changes

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

Show a summary per file
File Description
sdk/eventhubs/azure_messaging_eventhubs/tests/eventhubs_round_trip.rs Updated import for AmqpMessageProperties to reflect its new namespace.
sdk/eventhubs/azure_messaging_eventhubs/tests/eventhubs_producer.rs Adjusted import paths to the new message namespace format.
sdk/eventhubs/azure_messaging_eventhubs/src/models/mod.rs Moved AmqpMessageId to the message namespace in production and test code.
sdk/eventhubs/azure_messaging_eventhubs/src/models/event_data.rs Updated import for AmqpMessageProperties to reference the new location.
sdk/eventhubs/azure_messaging_eventhubs/src/consumer/mod.rs Reorganized imports to follow new module structure.
sdk/eventhubs/azure_messaging_eventhubs/benches/benchmarks.rs Modified benchmark configuration including improved test-mode handling and updated measurement times.
sdk/core/azure_core_amqp/src/value.rs Added documentation for AmqpComposite public methods.
sdk/core/azure_core_amqp/src/messaging.rs Adjusted import to use the new message namespace for AmqpMessageBody and made builders module public within the crate.
sdk/core/azure_core_amqp/src/lib.rs Reorganized module visibility and re-exported the builder and message modules.
sdk/core/azure_core_amqp/src/fe2o3/messaging/mod.rs Refined imports for AmqpMessageProperties from the messaging module.
sdk/core/azure_core_amqp/src/fe2o3/error.rs Updated conversion use for AmqpSymbol.
sdk/core/azure_core_amqp/src/error.rs Cleaned up unused imports and commented code.
Comments suppressed due to low confidence (1)

sdk/eventhubs/azure_messaging_eventhubs/benches/benchmarks.rs:141

  • The measurement time for the send_benchmarks group has been increased significantly from 205 seconds to 2200 seconds. Please verify that this change is intentional and that the extended benchmark duration will not adversely affect overall test suite runtime.
.measurement_time(std::time::Duration::new(2200, 0));

@LarryOsterman LarryOsterman enabled auto-merge (squash) May 15, 2025 23:43
@LarryOsterman LarryOsterman merged commit a9a5544 into Azure:main May 16, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core The azure_core crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants