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

Fix message IDs settlement order #11560

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Fix message IDs settlement order #11560

merged 1 commit into from
Jun 26, 2024

Conversation

ansd
Copy link
Member

@ansd ansd commented Jun 25, 2024

What?

This commit fixes issues that were present only on main branch and were introduced by #9022.

  1. Classic queues (specifically rabbit_queue_consumers:subtract_acks/3) expect message IDs to be (n)acked in the order as they were delivered to the channel / session proc. Hence, the lists:usort(MsgIds0) in rabbit_classic_queue:settle/5 was wrong causing not all messages to be acked adding a regression to also AMQP 0.9.1.
  2. The order in which the session proc requeues or rejects multiple message IDs at once is important. For example, if the client sends a DISPOSITION with first=3 and last=5, the message IDs corresponding to delivery IDs 3,4,5 must be requeued or rejected in exactly that order. For example, quorum queues use this order of message IDs in
    % Publishing to dead-letter exchange must maintain same order as messages got rejected.
    DiscardMsgs = lists:filtermap(fun(Id) ->
    case maps:get(Id, Checked, undefined) of
    undefined ->
    false;
    Msg ->
    {true, Msg}
    end
    end, MsgIds),
    to dead letter in that order.

How?

The session proc will settle (internal) message IDs to queues in ascending (AMQP) delivery ID order, i.e. in the order messages were sent to the client and in the order messages were settled by the client.

This commit chooses to keep the session's outgoing_unsettled_map map data structure.

An alternative would have been to use a queue or lqueue for the outgoing_unsettled_map as done in

Whether a queue (as done by rabbit_channel) or a map (as done by rabbit_amqp_session) performs better depends on the pattern how clients ack messages.

A queue will likely perform good enough because usually the oldest delivered messages will be acked first.
However, given that there can be many different consumers on an AQMP 0.9.1 channel or AMQP 1.0 session, this commit favours a map because it will likely generate less garbage and is very efficient when for example a single new message (or few new messages) gets acked while many (older) messages are still checked out by the session (but by possibly different AMQP 1.0 receivers).

 ## What?

This commit fixes issues that were present only on `main`
branch and were introduced by #9022.

1. Classic queues (specifically `rabbit_queue_consumers:subtract_acks/3`)
   expect message IDs to be (n)acked in the order as they were delivered
   to the channel / session proc.
   Hence, the `lists:usort(MsgIds0)` in `rabbit_classic_queue:settle/5`
   was wrong causing not all messages to be acked adding a regression
   to also AMQP 0.9.1.
2. The order in which the session proc requeues or rejects multiple
   message IDs at once is important. For example, if the client sends a
   DISPOSITION with first=3 and last=5, the message IDs corresponding to
   delivery IDs 3,4,5 must be requeued or rejected in exactly that
   order.
   For example, quorum queues use this order of message IDs in
   https://github.com/rabbitmq/rabbitmq-server/blob/34d3f943742bdcf7d34859edff8d45f35e4007d4/deps/rabbit/src/rabbit_fifo.erl#L226-L234
   to dead letter in that order.

 ## How?

The session proc will settle (internal) message IDs to queues in ascending
(AMQP) delivery ID order, i.e. in the order messages were sent to the
client and in the order messages were settled by the client.

This commit chooses to keep the session's outgoing_unsettled_map map
data structure.

An alternative would have been to use a queue or lqueue for the
outgoing_unsettled_map as done in
* https://github.com/rabbitmq/rabbitmq-server/blob/34d3f943742bdcf7d34859edff8d45f35e4007d4/deps/rabbit/src/rabbit_channel.erl#L135
* https://github.com/rabbitmq/rabbitmq-server/blob/34d3f943742bdcf7d34859edff8d45f35e4007d4/deps/rabbit/src/rabbit_queue_consumers.erl#L43

Whether a queue (as done by `rabbit_channel`) or a map (as done by
`rabbit_amqp_session`) performs better depends on the pattern how
clients ack messages.

A queue will likely perform good enough because usually the oldest
delivered messages will be acked first.
However, given that there can be many different consumers on an AQMP
0.9.1 channel or AMQP 1.0 session, this commit favours a map because
it will likely generate less garbage and is very efficient when for
example a single new message (or few new messages) gets acked while
many (older) messages are still checked out by the session (but by
possibly different AMQP 1.0 receivers).
@ansd ansd added the bug label Jun 25, 2024
@mergify mergify bot added the bazel label Jun 25, 2024
@ansd ansd marked this pull request as ready for review June 25, 2024 14:15
@ansd ansd merged commit dcb2fe0 into main Jun 26, 2024
251 checks passed
@ansd ansd deleted the classic-queue-settle-order branch June 26, 2024 11:35
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.

1 participant