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

[Service Bus] Filling the gaps for AmqpAnnotatedMessage #14786

Merged
10 commits merged into from
Apr 15, 2021

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Apr 7, 2021

#14703 missed a few things while translating Annotated message into RheaMessage.
This PR attempts to fill the gaps.

@HarshaNalluru HarshaNalluru changed the title Harshan/sb/issue/annotated fix [Service Bus] Filling the gaps for AmqpAnnotatedMessage Apr 7, 2021
@HarshaNalluru HarshaNalluru requested a review from yunhaoling April 8, 2021 20:59
body: encoder.encode(msg.body, msg.bodyType ?? "data"),
message_annotations: {}
return {
...AmqpAnnotatedMessage.toRheaMessage(msg),
Copy link
Member Author

Choose a reason for hiding this comment

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

We have custom logic for absolute_expiry_time for ServiceBusMessage
image

...we should do the same for AmqpAnnotatedMessage? (.NET has the same logic too!)

@JoshLove-msft

Copy link
Member Author

Choose a reason for hiding this comment

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

Pinging @yunhaoling @hemanttanwar to make sure the same logic is present in Java/Python too.

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't have this in python, for service bus message (and annotated message), we don't calculate absolute_expiry_time based on ttl.

two questions:

  1. during which process is your logic applied, setting the time_to_live property of a service bus message?
  2. absolute_expiry_time is in the amqp message property while ttl is in the amqp message header, I read the amqp spec but didn't find the spec saying those two are related to each other, so do you know why you have this logic?

ttl, Duration in milliseconds for which the message is to be considered "live". If this is set then a message expiration time will be computed based on the time of arrival at an intermediary. Messages that live longer than their expiration time will be discarded (or dead lettered). When a message is transmitted by an intermediary that was received with a ttl, the transmitted message's header SHOULD contain a ttl that is computed as the difference between the current time and the formerly computed message expiration time, i.e., the reduced ttl, so that messages will eventually die if they end up in a delivery loop.
absolute-expiry-time, An absolute time when this message is considered to be expired.

Copy link
Member Author

@HarshaNalluru HarshaNalluru Apr 9, 2021

Choose a reason for hiding this comment

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

during which process is your logic applied, setting the time_to_live property of a service bus message?

This gets applied to every message that is being sent(during the translation of SBMessage to the internal AMQP message).

absolute_expiry_time is in the amqp message property while ttl is in the amqp message header, I read the amqp spec but didn't find the spec saying those two are related to each other, so do you know why you have this logic?

Not from the spec, service-bus service has a bug that required us to add this(/cc @JoshLove-msft). Clemens is also involved here, it's better to reach out to him and get approval if we're changing anything here.

For JS especially,

  • SBMessage
    • doesn't expose the absolute_expiry_time on the message to be sent(and we use the custom logic to set the absolute_expiry_time property while translating it to the internal amqp message)
  • AmqpAnnotatedMessage
    • While sending, if we apply the same logic, we're overriding the property set by the user
    • Maybe we can consider removing CreationTime/AbsoluteExpiry from the AmqpAnnotatedMessage that is used for sending and go ahead with the custom logic of calculating it from ttl.

body: `message body ${randomTag}`,
bodyType: "data",
header: {
deliveryCount: 10, // TODO: Doesn't make sense to set on the message to be sent, should this be removed for sending?
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove deliveryCount from the AmqpAnnotatedMessage for send side?

Copy link
Member

Choose a reason for hiding this comment

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

Hm....this is an interesting problem we don't have with ServiceBusMessage, now that I think about it, since we make a distinction between a sendable message and a received message.

For now I'd say it's fine. A received AmqpAnnotatedMessage should also be immediately sendable so it should work, and we can just do a quick check to make sure that we aren't in some way dependent on those fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JoshLove-msft @yunhaoling @hemanttanwar what are you guys doing here?

propThree: true,
propFour: Date()
},
// deliveryAnnotations - TODO: should this be removed for sending?
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove deliveryAnnotations from the AmqpAnnotatedMessage for send side?

@HarshaNalluru HarshaNalluru marked this pull request as ready for review April 8, 2021 23:03
@HarshaNalluru HarshaNalluru force-pushed the harshan/sb/issue/annotated-fix branch from 85c4cf9 to 4eedc8d Compare April 9, 2021 06:43
Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Looks good to me

body: `message body ${randomTag}`,
bodyType: "data",
header: {
deliveryCount: 10, // TODO: Doesn't make sense to set on the message to be sent, should this be removed for sending?
Copy link
Member

Choose a reason for hiding this comment

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

Hm....this is an interesting problem we don't have with ServiceBusMessage, now that I think about it, since we make a distinction between a sendable message and a received message.

For now I'd say it's fine. A received AmqpAnnotatedMessage should also be immediately sendable so it should work, and we can just do a quick check to make sure that we aren't in some way dependent on those fields.

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Looks good. Merge it and I'll follow up on any of the open threads.

@ghost
Copy link

ghost commented Apr 14, 2021

Hello @HarshaNalluru!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit d6e35b8 into Azure:master Apr 15, 2021
@HarshaNalluru HarshaNalluru deleted the harshan/sb/issue/annotated-fix branch April 15, 2021 20:58
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
Azure#14703 missed a few things while translating Annotated message into RheaMessage.
This PR attempts to fill the gaps.
This pull request was closed.
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.

3 participants