raft topology: send cdc generation data in parts #13962
Merged
Conversation
This file contains 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
|
@kbr-scylla please review |
|
CI state |
kbr-scylla
requested changes
May 22, 2023
|
Please also rebase the PR on |
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
5ac7640
to
e3d341d
Compare
|
v2:
|
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
2 times, most recently
from
3c91bc8
to
d2fd8ca
Compare
|
v3:
|
|
CI state |
kbr-scylla
requested changes
May 28, 2023
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
d2fd8ca
to
4c65a94
Compare
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
4c65a94
to
498e090
Compare
|
v4:
v5:
|
|
CI state |
kbr-scylla
reviewed
Jun 1, 2023
|
@gleb-cloudius @piodul please review |
What happens if one command succeeds and the next fails? Do we end up in an inconsistent state? |
piodul
reviewed
Jun 1, 2023
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
498e090
to
8620d19
Compare
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
8620d19
to
5697dc1
Compare
kbr-scylla
approved these changes
Jul 5, 2023
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
4bfe367
to
744599f
Compare
|
@scylladb/scylla-maint please review and trigger CI for this pull request |
|
v11:
|
kbr-scylla
approved these changes
Jul 5, 2023
|
CI state |
Looks like the second node booted by the test hanged due to topology coordinator errors: |
kbr-scylla
requested changes
Jul 5, 2023
There was a problem hiding this comment.
Please fix the order of commits due to the reasons explained by Piotr here: #13962 (comment)
and please investigate the failure.
Provide useful description for `write_mutations` and `broadcast_tables_query` that is stored in `system.group0_history`. Reduces scope of issue scylladb#13370.
For now, `raft_sys_table_storage::_max_mutation_size` equals `max_mutation_size` (half of the commitlog segment size), so with some additional information, it can exceed this threshold resulting in throwing an exception when writing mutation to the commitlog. A batch of raft commands has the size at most `group0_state_machine::merger::max_command_size` (half of the commitlog segment size). It doesn't have additional metadata, but it may have a size of exactly `max_mutation_size`. It shouldn't make any trouble, but it is prefered to be careful. Make `raft_sys_table_storage::_max_mutation_size` and `group0_state_machine::merger::max_command_size` more strict to leave space for metadata. Fixed typo "1204" => "1024".
…_size` `get_cdc_generation_mutations` splits data to mutations of maximal size `mutation_size_treshold`. Before this commit it was hardcoded to 2 MB. Calculate `mutation_size_threshold` to leave space for cdc generation data and not exceed `max_command_size`.
…ion_mutations snapshot Topology snapshots contain only mutation of current CDC generation data but don't contain any previous or future generations. If new a generation of data is being broadcasted but hasn't been entirely applied yet, the applied part won't be sent in a snapshot. In this scenario, new or delayed nodes can never get the applied part. Send entire cdc_generations_v3 table in the snapshot to resolve this problem. As a follow-up, a mechanism to remove old CDC generations will be introduced.
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
744599f
to
d3eafbf
Compare
|
v12:
|
… from all merged commands If `group0_state_machine` applies all commands individually (without batching), the resulting current `state_id` -- which will be compared with the `prev_state_id` of the next command if it is a guarded command -- equals the maximum of the `next_state_id` of all commands applied up to this point. That's because the current `state_id` is obtained from the history table by taking the row with the largest clustering key. When `group0_state_machine::apply` is called with a batch of commands, the current `state_id` is loaded from `system.group0_history` to `merger::last_group0_state_id` only once. When a command is merged, its `next_state_id` overwrites `last_group0_state_id`, regardless of their order. Let's consider the following situation: The leader sends two unguarded `write_mutations` commands concurrently, with timeuuids T1 and T2, where T1 < T2. Leader waits to apply them and sends guarded `topology_change` with `prev_state_id` equal T2. Suppose that the command with timeuuid T2 is committed first, and these commands are small enough that all of `write_mutations` could be merged into one command. Some followers can get all of these three commands before its `fsm` polls them. In this situation, `group0_state_machine::apply` is called with all three of them and `merger` will merge both `write_mutations` into one command. After that, `merger::last_group0_state_id` will be equal to T1 (this command was committed as the second one). When it processes the `topology_change` command, it will compare its `prev_state_id` and `merger::last_group0_state_id`, resulting in making this command a no-op (which wouldn't happen if the commands were applied individually). Such a scenario results in inconsistent results: one replica applies `topology_change`, but another makes it a no-op.
This function takes guard and calls its destructor. It's used to not call raw destructor.
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
a61cd5d
to
6f3a00d
Compare
Broadcasts all mutations returned from `prepare_new_cdc_generation_data` except the last one. Each mutation is sent in separate raft command. It takes `group0_guard`, and if the number of mutations is greater than one, the guard is dropped, and a new one is created and returned, otherwise the old one will be returned. Commands are sent in parallel and unguarded (the guard used for sending the last mutation will guarantee that the term hasn't been changed). Returns the generation's UUID, guard and last mutation, which will be sent with additional topology data by the caller. If we send the last mutation in the `write_mutation` command, we would use a total of `n + 1` commands instead of `n-1 + 1` (where `n` is the number of mutations), so it's better to send it in `topology_change` (we need to send it after all `write_mutations`) with some small metadata. With the default commitlog segment size, `mutation_size_threshold` will be 4 MB. In large clusters e.g. 100 nodes, 64 shards per node, 256 vnodes cdc generation data can reach the size of 30 MB, thus there will be no more than 8 commands. In a multi-DC cluster with 100ms latencies between DCs, this operation should take about 200ms since we send the commands concurrently, but even if the commands were replicated sequentially by Raft, it should take no more than 1.6s, which is incomparably smaller than bootstrapping operation (bootstrapping is quick if there is no data in the cluster, but usually if one has 100 nodes they have tons of data, so indeed streaming/repair will take much longer (hours/days)). Fixes FIXME in pr scylladb#13683.
…_unknown` exception in topology coordinator loop When the topology_cooridnator fiber gets `raft::commit_status_unknown`, it prints an error. This exception is not an error in this case, and it can be thrown when the leader has changed. It can happen in `add_entry_unguarded` while sending a part of the CDC generation data in the `write_mutations` command. Catch this exception in `topology_coordinator::run` and print a warning.
This test limits `commitlog_segment_size_in_mb` to 2, thus `max_command_size`
is limited to less than 1 MB. It adds an injection which copies mutations
generated by `get_cdc_generation_mutations` n times, where n is picked that
the memory size of all mutations exceeds `max_command_size`.
This test passes if cdc generation data is committed by raft in multiple commands.
If all the data is committed in a single command, the leader node will loop trying
to send raft command and getting the error:
```
storage_service - raft topology: topology change coordinator fiber got error raft::command_is_too_big_error (Command size {} is greater than the configured limit {})
```
margdoc
force-pushed
the
raft_topology_cdc_generation_data_in_parts
branch
from
6f3a00d
to
4e3c97d
Compare
kbr-scylla
approved these changes
Jul 7, 2023
|
CI state |
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.
The CDC generation data can be large and not fit in a single command. This pr splits it into multiple mutations by smartly picking a
mutation_size_thresholdand sending each mutation as a separate group 0 command.Commands are sent sequentially to avoid concurrency problems.
Topology snapshots contain only mutation of current CDC generation data but don't contain any previous or future generations. If a new generation of data is being broadcasted but hasn't been entirely applied yet, the applied part won't be sent in a snapshot. New or delayed nodes can never get the applied part in this scenario.
Send the entire cdc_generations_v3 table in the snapshot to resolve this problem.
A mechanism to remove old CDC generations will be introduced as a follow-up.