Skip to content

rabbit_fifo: Activate next SAC deterministically #14100

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

Closed
wants to merge 1 commit into from

Conversation

the-mikedavis
Copy link
Collaborator

This change extends the changes in #13971 (making rabbit_fifo deterministic). Determining the next active consumer uses a maps:iterator() which has undefined ordering. Like calls to fold/3 or keys/1 we should ensure ordering of the iterator so that two replicas are guaranteed to find the same consumer.

@the-mikedavis the-mikedavis requested review from kjnilsson and ansd June 20, 2025 15:10
@the-mikedavis the-mikedavis self-assigned this Jun 20, 2025
Comment on lines -32 to +33
Iterable = case is_deterministic(Vsn) of
true ->
maps:iterator(Map, ordered);
false ->
Map
end,
maps:fold(Fun, Init, Iterable).
maps:fold(Fun, Init, iterator(Map, Vsn)).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that maps:fold/3 creates an iterator with maps:iterator/1 by default, so this is equivalent to the old code which passed a map directly: https://github.com/erlang/otp/blob/d4c78e0414664564ae29d4d6a09bbaf7d2c2110b/lib/stdlib/src/maps.erl#L881-L882

This change extends the changes in 2db4843 (making `rabbit_fifo`
deterministic). Determining the next active consumer uses a
`maps:iterator()` which has undefined ordering. Like calls to `fold/3`
or `keys/1` we should ensure ordering of the iterator so that two
replicas are guaranteed to find the same consumer.
@the-mikedavis the-mikedavis force-pushed the md/qq-deterministic-sac branch from 864d5c4 to 02fc85c Compare June 20, 2025 15:21
Copy link
Member

@ansd ansd left a comment

Choose a reason for hiding this comment

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

Thank you! :)

I left this part out intentionally in #13971 because there can be only one single active consumer. So, it shouldn't matter in which order the map is iterated? And because there is a little performance benefit if the iteration order is undefined (it seems to be called on the hot path in complete_and_checkout/6), I decided against it.

@the-mikedavis
Copy link
Collaborator Author

Aha I see now, this path is only used for the single-active consumer. I think I see now why @kjnilsson mentioned that using an iterator here is odd 😅. I assume it makes sense performance-wise to find the SAC this way.

I'll close this out then - this is already deterministic since there can be only one SAC. Thanks @ansd!

@the-mikedavis the-mikedavis deleted the md/qq-deterministic-sac branch June 20, 2025 16:39
@ansd
Copy link
Member

ansd commented Jun 20, 2025

Yes 🙂 Thank you though for following because it's always good to double check these different places where map traversal with undefined order in the Ra machine's apply callback takes place. For example, there are also some places in https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbit/src/rabbit_stream_coordinator.erl and https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbit/src/rabbit_stream_sac_coordinator.erl which use undefined map iteration and therefore look non-deterministic, however we discussed internally that they are all fine. JSP also checked the Ra state machine in Khepri IIRC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants