ra_server_proc: Fix handling of local query replies #517
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dumbbell
force-pushed
the
fix-delayed-query-handling
branch
from
1cb588f to
a8b8a75
Compare
[Why]
The reply of local query was formatted as:
{reply, From, Reply}
However, the code was called in two different contexts:
1. When the local query was executed immediately, the reply tuple was
interpreted by `gen_statem` and thus the reply was sent by the local
Ra server process. Exactly what we wanted.
2. When the local query execution depended on a condition and might have to be
delayed, it could be executed right away (and the reply tuple was
interpreted by `gen_statem`), or the execution could be delayed and
at the time of execution, the reply tuple could be interpreted as a
Ra effect.
Unfortunately, the same tuple has a very different meaning depending on
who interprets it:
* `gen_statem` will send the reply regardless of the Raft state of the
Ra server process.
* The Ra effect will only send the reply if the Ra server process is
acting as the leader.
The delayed query was always processed by the Ra server that received
it, regardless of its state. This means that if the Ra server is not a
leader and the reply tuple is interpreted as a Ra effect, the caller
will never get an answer.
This led to some timeouts in Khepri and nasty bugs in RabbitMQ. In
particular, it caused the `peer_discovery_classic_config_SUITE` to fail
quite frequently in RabbitMQ CI.
[How]
First, the patch ensures the reply tuple is always interpreted as a Ra effect.
Then, it sets the `{member, ...}` replier option in the reply effect to
the Ra server that executes the query. This way, the reply is always
emitted emitted and by the correct Ra server, regardless of its Raft
state.
V2: Modify the existing testcase to make sure a local query sent to the
follower that should block reaches the follower before it can
fulfill it. Indeed, previously, that test against the follower would
be serially after the same test against the leader. Therefore, the
blocking would happen with the leader and when it was the turn of
the follower, it would be able to execute the query right away.
Now, both tests are executed in parallel, with timers to ensure the
follower gets the query first, way before the command it is waiting
for is sent.
This modified testcase was tested without the fix and it
demonstrated the problem successfully; in other words, the testcase
failed 100% of the time without the fix.
dumbbell
force-pushed
the
fix-delayed-query-handling
branch
from
a8b8a75 to
0100c43
Compare
kjnilsson
approved these changes
Mar 12, 2025
dumbbell
added a commit
to rabbitmq/khepri
that referenced
this pull request
Mar 13, 2025
Release notes: * https://github.com/rabbitmq/ra/releases/tag/v2.16.1 * https://github.com/rabbitmq/ra/releases/tag/v2.16.2 * https://github.com/rabbitmq/ra/releases/tag/v2.16.3 In particular, it fixes a bug with the handling of delayed queries' reply (rabbitmq/ra#517). This caused some timeouts, especially in `khepri_machine:fence/2`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
The reply of local query was formatted as:
{reply, From, Reply}However, the code was called in two different contexts:
When the local query was executed immediately, the reply tuple was interpreted by
gen_statemand thus the reply was sent by the local Ra server process. Exactly what we wanted.When the local query execution depended on a condition and might have to be delayed, it could be executed right away (and the reply tuple was interpreted by
gen_statem), or the execution could be delayed and at the time of execution, the reply tuple could be interpreted as a Ra effect.Unfortunately, the same tuple has a very different meaning depending on who interprets it:
gen_statemwill send the reply regardless of the Raft state of the Ra server process.The delayed query was always processed by the Ra server that received it, regardless of its state. This means that if the Ra server is not a leader and the reply tuple is interpreted as a Ra effect, the caller will never get an answer.
This led to some timeouts in Khepri and nasty bugs in RabbitMQ. In particular, it caused the
peer_discovery_classic_config_SUITEto fail quite frequently in RabbitMQ CI.How
First, the patch ensures the reply tuple is always interpreted as a Ra effect.
Then, it sets the
{member, ...}replier option in the reply effect to the Ra server that executes the query. This way, the reply is always emitted emitted and by the correct Ra server, regardless of its Raft state.