Shovel: convert AMQP 1.0 props and app props to AMQP 0.9.1 props and headers

[luos]

Hi,

We've implemented Application Property conversion to AMQP 0.9.1 headers for shovel. We wanted this to work on 3.12 and we were not sure if this is appropriate for these message container changes.

Let us know what you think.

• Timestamps are milliseconds in AMQP 1.0, but in AMQP 0.9.1 it is seconds. Fixed by multiplying the timestamp by 1 000.
• Shovel crashed if user_id was set in the message because the encoding was as utf8 while it should be a byte array.
• Negative integers were encoded as integers - therefore leading to incorrect positive values.
• Float values were not supported by the client.
• Fixed priority header encoding in AMQP 1.0. It was set as uint but it should be ubyte.
• Priority of the message is now in the Headers instead of Application Properties. This is potentially a breaking change.

GitHub issue: https://github.com/rabbitmq/rabbitmq-server/issues/7508.

A v3.12.x version of this PR: https://github.com/rabbitmq/rabbitmq-server/pull/10037.

Proposed Changes

Implement conversion of Application Properties to Headers.

Configurable by using the following key:

shovel.convert_amqp10_props_to_amqp091 = true

Types of Changes

What types of changes does your code introduce to this project? Put an x in the boxes that apply

• [ ] Bug fix (non-breaking change which fixes issue #NNNN)
• [x] New feature (non-breaking change which adds functionality)
• [x] Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• [ ] Documentation improvements (corrections, new content, etc)
• [ ] Cosmetic change (whitespace, formatting, etc)
• [ ] Build system and/or CI

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask on the mailing list. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• [x] I have read the CONTRIBUTING.md document
• [x] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• [x] I have added tests that prove my fix is effective or that my feature works
• [x] All tests pass locally with my changes
• [ ] If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
• [ ] If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

[michaelklishin]

Message containers were introduced in part to make this kind of conversions easier to pull off.

[michaelklishin]

@luos why was the priority value moved to a header in AMQP 1.0? I doubt we can ship this in 3.12.x because of this change.

[luos]

Hi,

I think we can definitely remove the priority change, I changed this because there is already a place for it in the AMQP 1.0 Headers and it gets placed into Application Properties. This is not critical for us.

Regarding the MC, I'd be happy to make this more MC compatible, though I am quite unsure how as shovels are a level above that if I understand it correctly, or if somehow we can make this in a way that in the future it can be easily changed to use MC?

[michaelklishin]

Ah, so it is a header in AMQP 1.0 (section 3.2.1). Then I guess it should be acceptable.

[michaelklishin]

@luos for Shovels specifically this can remain where it is. Larger changes can come in Shovel v2 for which we have a lot of other internal changes.

[michaelklishin]

@luos we can drop the configuration key and treat this as a bug fix for Shovels with AMQP 1.0 destinations.

[luos]

We prepared the backporting here: https://github.com/rabbitmq/rabbitmq-server/pull/10037

We will work on the MC version at the end of this week.

[luos]

Hi @michaelklishin ,

We've started looking at the conversion with MC. We are planning to remove the Props and Payload from the rabbit_shovel_behaviour:forward function, and replace it with an mc "object".

We see that there is a somewhat related PR already which contains some AMQP 1.0 MC conversion functions, we may depend on these changes. What would be your preference, should we base our changes on that PR (#9022) by @ansd ?

https://github.com/rabbitmq/rabbitmq-server/pull/9022/files#diff-fb45468cd5ac6820b904a3e908da68af368f0e70126a7df503efedc940e0d03b

[michaelklishin]

@luos #9022 is vast in scope and while you can coordinate your changes with it, it is something for 4.0.

[michaelklishin]

@kjnilsson @ansd this must be the first use of mc outside of core and outside of the core team. WDYT? Should Shovels simply do what they do in v3.12.x, at least for now?