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

Add condition option to local queries #450

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

dumbbell
Copy link
Member

@dumbbell dumbbell commented Jun 20, 2024

Why

It allows to wait for a condition to become true before the query can be executed.

The condition we need right now is to wait for an index to be applied locally (or on the leader). It is useful when the caller wants to be sure that the result of the previous command is "visible" by the next query.

By default, it's not guarantied because the command will be considered successfully applied as long as a quorum of Ra servers applied it. This list of Ra servers may not include the local node for instance.

How

If the condition option is specified with a {applied, {Index, Term}} tuple, the query will be evaluated right away if that index is already applied, or it will be added to a list of pending queries.

Pending queries are evaluated after each applied batch of commands by the local node. If a pending query's target index was reached or passed, it is evaluated. If a pending query's target term ended, an error is returned.

Note that pending queries that timed out from the callers' point of view will still be evaluated once their associated condition becomes true. The reply will be discarded by Erlang however because the process alias will be inactivate at that point.

Here is an example:

ra:local_query(ServerId, QueryFun, #{condition => {applied, {Index, Term}}}).

The local_query tuple sent to the Ra server changes format. The old one was:

{local_query, QueryFun}

The new one is:

{local_query, QueryFun, Options}

If the remote Ra server that receives the query runs a version of Ra older than the one having this change and thus doesn't understand the new tuple, it will ignore and drop the query. This will lead to a timeout of the query, or an indefinitely hanging call if the timeout was set to infinity.

Note in the opposite situation, i.e. if a Ra server that knows the new query tuple receives an old tuple, it will evaluate the query as if the options was an empty map.

V2: Rename the option from limit to wait_for_index which is more explicit.
V3: Rename the option back to limit. It allows to pass other types of condition in the future. Also change the place where pending queries are evaluated. This allows to get rid of the applied_to effect.
V4: Rename the option to condition to make its purpose more intuitive. The value was changed to {applied, {Index, Term} to give more meaning to what the condition does. While here, the ra_idxterm() type is aliased to idxterm() and exported as ra:idxterm().

@dumbbell dumbbell self-assigned this Jun 20, 2024
@dumbbell
Copy link
Member Author

After submitting the pull request, I admit the parameter name limt isn't really self-explanatory.

Here is what the call looks like:

ra:local_query(ServerId, QueryFun, #{limit => {Index, Term}}).

What about wait_for or even wait_for_index instead? Example:

ra:local_query(ServerId, QueryFun, #{wait_for_index => {Index, Term}}).

Any opinion?

@dumbbell dumbbell force-pushed the add-local-query-limit-option branch from 8764db6 to 95b3629 Compare June 20, 2024 15:26
@dumbbell dumbbell requested a review from kjnilsson June 20, 2024 15:27
@dumbbell dumbbell force-pushed the add-local-query-limit-option branch from 95b3629 to 820cd0f Compare June 24, 2024 13:32
@dumbbell dumbbell changed the title Add limit option to local queries Add wait_for_index option to local queries Jun 24, 2024
@dumbbell
Copy link
Member Author

I renamed the option from limit to wait_for_index.

@dumbbell dumbbell force-pushed the add-local-query-limit-option branch from 820cd0f to f4211b5 Compare June 24, 2024 13:50
@dumbbell dumbbell marked this pull request as ready for review June 24, 2024 14:17
@dumbbell dumbbell force-pushed the add-local-query-limit-option branch from f4211b5 to 10a1f9d Compare June 25, 2024 14:28
src/ra_server_proc.erl Outdated Show resolved Hide resolved
src/ra.erl Outdated Show resolved Hide resolved
src/ra_server.erl Outdated Show resolved Hide resolved
@dumbbell dumbbell force-pushed the add-local-query-limit-option branch 2 times, most recently from f340066 to 7bfec04 Compare June 26, 2024 13:29
@kjnilsson kjnilsson changed the title Add wait_for_index option to local queries Add condition option to local queries Jun 26, 2024
@dumbbell dumbbell force-pushed the add-local-query-limit-option branch from 7bfec04 to f3a984b Compare June 26, 2024 13:51
[Why]
It allows to wait for a condition to become true before the query can be
executed.

The condition we need right now is to wait for an index to be applied
locally (or on the leader). It is useful when the caller wants to be
sure that the result of the previous command is "visible" by the next
query.

By default, it's not guarantied because the command will be considered
successfully applied as long as a quorum of Ra servers applied it. This
list of Ra servers may not include the local node for instance.

[How]
If the `condition` option is specified with a `{applied, {Index, Term}}`
tuple, the query will be evaluated right away if that index is already
applied, or it will be added to a list of pending queries.

Pending queries are evaluated after each applied batch of commands by
the local node. If a pending query's target index was reached or passed,
it is evaluated. If a pending query's target term ended, an error is
returned.

Note that pending queries that timed out from the callers' point of view
will still be evaluated once their associated condition becomes true.
The reply will be discarded by Erlang however because the process alias
will be inactivate at that point.

Here is an example:

    ra:local_query(ServerId, QueryFun, #{condition => {applied, {Index, Term}}}).

The `local_query` tuple sent to the Ra server changes format. The old
one was:

    {local_query, QueryFun}

The new one is:

    {local_query, QueryFun, Options}

If the remote Ra server that receives the query runs a version of Ra
older than the one having this change and thus doesn't understand the
new tuple, it will ignore and drop the query. This will lead to a
timeout of the query, or an indefinitely hanging call if the timeout was
set to `infinity`.

Note in the opposite situation, i.e. if a Ra server that knows the new
query tuple receives an old tuple, it will evaluate the query as if the
options was an empty map.

V2: Rename the option from `limit` to `wait_for_index` which is more
    explicit.
V3: Rename the option back to `limit`. It allows to pass other types of
    condition in the future.
    Also change the place where pending queries are evaluated. This
    allows to get rid of the `applied_to` effect.
V4: Rename the option to `condition` to make its purpose more intuitive.
    The value was changed to `{applied, {Index, Term}` to give more
    meaning to what the condition does.
    While here, the `ra_idxterm()` type is aliased to `idxterm()` and
    exported as `ra:idxterm()`.
@dumbbell dumbbell force-pushed the add-local-query-limit-option branch from f3a984b to a414eb5 Compare June 26, 2024 15:03
@kjnilsson kjnilsson merged commit 776eaf7 into main Jun 26, 2024
10 checks passed
@dumbbell dumbbell added this to the 2.12.0 milestone Jul 9, 2024
@dumbbell dumbbell deleted the add-local-query-limit-option branch July 9, 2024 15:16
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.

None yet

2 participants