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

Changed AMQP message receive model to require explicit delivery accept/reject. #1974

Merged

Conversation

LarryOsterman
Copy link
Member

Updated the infrastructure for receiving AMQP messages to require that message deliveries be explicitly accepted or rejected - this is not needed for Eventhubs but will be a requirement for ServiceBus.

This PR also includes an enhancement to the AMQP receiver to set and retrieve the link credit mode - this will almost certainly be needed to implement batch receive.

Please note that much of this infrastructure is not currently tested (and almost certainly doesn't work correctly) because we don't have full live test support.

This PR also includes the start of tests for AMQP connection APIs. but this functionality is disabled because we don't have CI pipeline support for starting the test AMQP broker.

Bonus gratuitous changes: I removed the word "test" from a number of test functions because it's redundant.

CoPilot summary:

This pull request includes several changes to the azure_core_amqp package, focusing on improving the handling of AMQP deliveries and enhancing the test infrastructure. The changes introduce new structs and traits for better abstraction and add new methods to existing traits. Additionally, some tests have been disabled temporarily until the CI environment can support them.

Improvements to AMQP delivery handling:

  • Introduced Fe2o3AmqpDelivery struct and implemented AmqpDeliveryApis trait for it in sdk/core/azure_core_amqp/src/fe2o3/messaging/messaging_types.rs.
  • Added AmqpDelivery struct and AmqpDeliveryApis trait in sdk/core/azure_core_amqp/src/messaging.rs to abstract delivery handling.

Enhancements to AMQP receiver:

  • Added methods for setting and getting credit mode, and handling delivery acceptance, rejection, and release in sdk/core/azure_core_amqp/src/fe2o3/receiver.rs. [1] [2]
  • Updated AmqpReceiverApis trait to include new delivery-related methods in sdk/core/azure_core_amqp/src/receiver.rs. [1] [2]

Test infrastructure improvements:

  • Disabled certain tests in sdk/core/azure_core_amqp/src/connection.rs until the AMQP test broker can be started in CI.
  • Renamed test functions for consistency in sdk/core/azure_core_amqp/src/connection.rs. [1] [2]

Additional changes:

  • Added tokio as a dev dependency in sdk/core/azure_core_amqp/Cargo.toml.
  • Updated imports and logging levels in sdk/core/azure_core_amqp/src/fe2o3/connection.rs.

These changes collectively enhance the robustness and maintainability of the azure_core_amqp package.

sdk/core/azure_core_amqp/src/connection.rs Show resolved Hide resolved
sdk/core/azure_core_amqp/src/fe2o3/connection.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/fe2o3/receiver.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/fe2o3/receiver.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/fe2o3/receiver.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/fe2o3/receiver.rs Outdated Show resolved Hide resolved
sdk/core/azure_core_amqp/src/noop.rs Outdated Show resolved Hide resolved
@LarryOsterman LarryOsterman requested a review from heaths January 8, 2025 19:49
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

I have a couple of suggestions, but otherwise LGTM.

sdk/core/azure_core_amqp/examples/connection.rs Outdated Show resolved Hide resolved
@LarryOsterman LarryOsterman merged commit e87e071 into Azure:main Jan 8, 2025
13 checks passed
@LarryOsterman LarryOsterman deleted the larryo/explicit_message_accept branch January 8, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants