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

[Azure.Core.Amqp] Lazy Allocation for All Sections #21791

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

jsquire
Copy link
Member

@jsquire jsquire commented Jun 10, 2021

Summary

The focus of these changes is to enable lazy allocation for all sections of the AmqpAnnotatedMessage type, to align with them being defined as optional by the AMQP specification. In order to allow downstream consumers to test if a section is populated without triggering allocation for the property, the HasSection member has been added.

References and Resources

@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Jun 10, 2021
@jsquire jsquire added this to the [2021] July milestone Jun 10, 2021
@jsquire jsquire self-assigned this Jun 10, 2021
@jsquire jsquire requested a review from tg-msft as a code owner June 10, 2021 20:16
@JoshLove-msft
Copy link
Member

LGTM. In terms of logistics, we still are planning on GAing the body types in next week or so. I'd rather wait to ship HasSection until we've implemented it in Event Hubs to prove that the API is correct.

@jsquire
Copy link
Member Author

jsquire commented Jun 10, 2021

LGTM. In terms of logistics, we still are planning on GAing the body types in next week or so. I'd rather wait to ship HasSection until we've implemented it in Event Hubs to prove that the API is correct.

No objections from me. To be clear, are we comfortable with shipping the body types from a commit or are you asking me to hold off on merging until after that point?

@JoshLove-msft
Copy link
Member

LGTM. In terms of logistics, we still are planning on GAing the body types in next week or so. I'd rather wait to ship HasSection until we've implemented it in Event Hubs to prove that the API is correct.

No objections from me. To be clear, are we comfortable with shipping the body types from a commit or are you asking me to hold off on merging until after that point?

I'd prefer to ship from tip of master. I'm not opposed to including this in the GA, but I would prefer to wait until we've done a validation of the API through Event Hubs.

@jsquire
Copy link
Member Author

jsquire commented Jun 10, 2021

I'd prefer to ship from tip of master. I'm not opposed to including this in the GA, but I would prefer to wait until we've done a validation of the API through Event Hubs.

I'll leave this open, then, until we ship.

@jsquire
Copy link
Member Author

jsquire commented Jun 17, 2021

/check-enforcer evaluate

The focus of these changes is to enable lazy allocation for all sections
of the `AmqpAnnotatedMessage` type, to match the AMQP spec definition.
(see: http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#section-message-format)

In order to allow downstream consumers to test if a section is populated
without triggering allocation for the property, the `HasSection` member
has been added.
@jsquire jsquire force-pushed the amqp/has-section branch from bf3bb29 to 3de1b87 Compare June 17, 2021 18:47
@jsquire jsquire merged commit a551ee5 into Azure:master Jun 17, 2021
@jsquire jsquire deleted the amqp/has-section branch June 17, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants