rabbitmq-server icon indicating copy to clipboard operation
rabbitmq-server copied to clipboard

improve credit handling and header conversion for AMQP 1.0 plugin

Open luos opened this issue 3 years ago • 3 comments

Hi,

Sorry this became a so big but I was trying to debug something and somehow everything fall into one change. I can cut it up if needed.

Let me know what you think about these changes.

The AMQP 1.0 plugin grants publishers 65000 credit to the senders by default. This PR makes the credit given configurable.

The AMQP 1.0 plugin grants credit when the message is received, not when it is placed into the queue. This means that an AMQP 1.0 sender who does not care about settlement can overload the receiver process leading to high memory usage.

The AMQP 1.0 plugin creates a classic queue by default. This is annoying because the client can not ensure that the target exists (it needs to use /amq/queue/Q target instead of the name of the queue. Now the plugin can be configured to use Quorum Queues.

Sending an unsettled message on a link set as 'settled' is now a protocol error. Before this change it was leading to crashes of the AMQP process with a crash of the link.

Implements optional conversion of AMQP 1.0 headers to 0.9.1 headers for some headers types (utf8, uint, bool). Implements #2591

Implements optional conversion of AMQP 0.9.1 headers to AMQP 1.0 App Properties.

Also fixes a bug in the amqp10_client where the client can run out of credit and get stuck because of bad accounting.

The tests are prepared to run with changes when https://github.com/rabbitmq/rabbitmq-server/issues/4832 is fixed, however they pass without it as well.

Types of Changes

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

  • [x] Bug fix (non-breaking change which fixes issue #NNNN)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] 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

Further Comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc.

luos avatar May 17 '22 14:05 luos

This pull request modifies the bazel build only. If it is a deps update or app_env change, remember to sync any changes to the makefiles.

mergify[bot] avatar May 17 '22 14:05 mergify[bot]

Hi yes I think it would be good to split this into multiple PRs:

  1. for credit related changes
  2. Message conversion related changes (ideally we want to consider rabbit_msg_record for all conversions).
  3. Quorum queue configurability
  4. Unsettled fix

I know it's a pain to split it but if we do this we can focus properly on each feature and merge earlier.

kjnilsson avatar May 19 '22 11:05 kjnilsson

Hi yes I think it would be good to split this into multiple PRs:

  1. for credit related changes
  2. Message conversion related changes (ideally we want to consider rabbit_msg_record for all conversions).
  3. Quorum queue configurability
  4. Unsettled fix

I know it's a pain to split it but if we do this we can focus properly on each feature and merge earlier.

kjnilsson avatar May 19 '22 11:05 kjnilsson

@luos what's next for this PR? can it be updated to only contain credit related fixes?

kjnilsson avatar Sep 27 '22 11:09 kjnilsson

Hi, yes, I just opened a new PR for it: #6273

luos avatar Oct 27 '22 15:10 luos