The function `gms::version_generator::get_next_version()` can only be called from shard 0 as it uses a global, unsynchronized counter to issue versions. Notably, the function is used as a default argument for the constructor of `gms::versioned_value` which is used from shorthand constructors such as `versioned_value::cache_hitrates`, `versioned_value::schema` etc.
The `cache_hitrate_calculator` service runs a periodic job which updates the `CACHE_HITRATES` application state in the local gossiper state. Each time the job is scheduled, it runs on the next shard (it goes through shards in a round-robin fashion). The job uses the `versioned_value::cache_hitrates` shorthand to create a `versioned_value`, therefore risking a data race if it is not currently executing on shard 0.
The PR fixes the race by moving the call to `versioned_value::cache_hitrates` to shard 0. Additionally, in order to help detect similar issues in the future, a check is introduced to `get_next_version` which aborts the process if the function was called on other shard than 0.
There is a possibility that it is a fix for #17493. Because `get_next_version` uses a simple incrementation to advance the global counter, a data race can occur if two shards call it concurrently and it may result in shard 0 returning the same or smaller value when called two times in a row. The following sequence of events is suspected to occur on node A:
1. Shard 1 calls `get_next_version()`, loads version `v - 1` from the global counter and stores in a register; the thread then is preempted,
2. Shard 0 executes `add_local_application_state()` which internally calls `get_next_version()`, loads `v - 1` then stores `v` and uses version `v` to update the application state,
3. Shard 0 executes `add_local_application_state()` again, increments version to `v + 1` and uses it to update the application state,
4. Gossip message handler runs, exchanging application states with node B. It sends its application state to B. Note that the max version of any of the local application states is `v + 1`,
5. Shard 1 resumes and stores version `v` in the global counter,
6. Shard 0 executes `add_local_application_state()` and updates the application state - again - with version `v + 1`.
7. After that, node B will never learn about the application state introduced in point 6. as gossip exchange only sends endpoint states with version larger than the previous observed max version, which was `v + 1` in point 4.
Note that the above scenario was _not_ reproduced. However, I managed to observe a race condition by:
1. modifying Scylla to run update of `CACHE_HITRATES` much more frequently than usual,
2. putting an assertion in `add_local_application_state` which fails if the version returned by `get_next_version` was not larger than the previous returned value,
3. running a test which performs schema changes in a loop.
The assertion from the second point was triggered. While it's hard to tell how likely it is to occur without making updates of cache hitrates more frequent - not to mention the full theorized scenario - for now this is the best lead that we have, and the data race being fixed here is a real bug anyway.
Refs: #17493Closesscylladb/scylladb#17499
* github.com:scylladb/scylladb:
version_generator: check that get_next_version is called on shard 0
misc_services: fix data race from bad usage of get_next_version
Since commit f1bbf70, many compaction types can do cleanup work, but turns out
we forgot to invalidate cache on their completion.
So if a node regains ownership of token that had partition deleted in its previous
owner (and tombstone is already gone), data can be resurrected.
Tablet is not affected, as it explicitly invalidates cache during migration
cleanup stage.
Scylla 5.4 is affected.
Fixes#17501.
Fixes#17452.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Closesscylladb/scylladb#17502
before this change, we rely on the default-generated fmt::formatter created from operator<<, but fmt v10 dropped the default-generated formatter.
in this change, we define formatters for
* canonical_mutation
* atomic_cell_view
* atomic_cell
* atomic_cell_or_collection::printer
Refs #13245Closesscylladb/scylladb#17506
* github.com:scylladb/scylladb:
mutation: add fmt::formatter for canonical_mutation
mutation: add fmt::formatter for atomic_cell_view and atomic_cell
mutation: add fmt::formatter for atomic_cell_or_collection::printer
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for canonical_mutation
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* atomic_cell_view
* atomic_cell
and drop their operator<<:s.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
`atomic_cell_or_collection::printer`, and drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
The get_next_version function can only be safely called from shard 0,
but this constraint is not enforced in any way. As evidenced in the
previous commit, it is easy to accidentally call it from a non-zero
shard.
Introduce a runtime check to get_next_version which calls
on_fatal_internal_error if it detects that the function was called form
the wrong shard. This will let us detect cross-shard use issues in
runtime.
The function `gms::version_generator::get_next_version()` can only be
called from shard 0 as it uses a global, unsynchronized counter to
issue versions. Notably, the function is used as a default argument for
the constructor of `gms::versioned_value` which is used from shorthand
constructors such as `versioned_value::cache_hitrates`,
`versioned_value::schema` etc.
The `cache_hitrate_calculator` service runs a periodic job which
updates the `CACHE_HITRATES` application state in the local gossiper
state. Each time the job is scheduled, it runs on the next shard (it
goes through shards in a round-robin fashion). The job uses the
`versioned_value::cache_hitrates` shorthand to create a
`versioned_value`, therefore risking a data race if it is not currently
executing on shard 0.
Fix the race by constructing the versioned value on shard 0.
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* wrapping_interval
* interval
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17488
Unfortunately, fmt v10 dropped support for operator<< formatters,
forcing us to replace the huge number of operator<< implementations
in our code by uglier and templated fmt::formatter implementations
to get Scylla to compile on modern distros (such as Fedora 39) :-(
Kefu has already started doing this migration, here is my small
contribution - the formatter for mutation_fragment_v2::kind.
This patch is need to compile, for example,
build/dev/mutation/mutation_fragment_stream_validator.o.
I can't remove the old operator<< because it's still used by
the implementation of other operator<< functions. We can remove
all of them when we're done with this coversion. In the meantime,
I replaced the original implementation of operator<< by a trivial
implementation just passing the work to the new fmt::print support.
Refs #13245
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#17432
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* bound_kind_m
* sstable_state
* indexable_element
* deletion_time
drop their operator<<:s
Refs #13245Closesscylladb/scylladb#17490
* github.com:scylladb/scylladb:
sstables: add fmt::formatter for deletion_time
sstable: add fmt::formatter for indexable_element
sstables: add fmt::foramtter for sstable_state
sstables: add fmt::formatter for sstables::bound_kind_m
before this change, we rely on the default-generated fmt::formatter created from operator<<, but fmt v10 dropped the default-generated formatter. in this change, we define formatters for some types used in testing.
Refs #13245Closesscylladb/scylladb#17485
* github.com:scylladb/scylladb:
test/unit: add fmt::formatter for tree_test_key_base
test: add printer for type for BOOST_REQUIRE_EQUAL
test: add fmt::formatters
test/perf: add fmt::formatters for scheduling_latency_measurer and perf_result
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* managed_bytes
* managed_bytes_view
* managed_bytes_opt
* occupancy_stats
and drop their operator<<:s
Refs https://github.com/scylladb/scylladb/issues/13245Closesscylladb/scylladb#17462
* github.com:scylladb/scylladb:
utils/managed_bytes: add fmt::formatters for managed_bytes and friends
utils/logalloc: add fmt::formatter for occupancy_stats
If index_reader isn't closed before it is destroyed, then ongoing
sstables reads won't be awaited and assertion will be triggered.
Close index_reader in has_partition_key before destroying it.
Fixes: #17232.
Closesscylladb/scylladb#17355
* github.com:scylladb/scylladb:
test: add test to check if reader is closed
sstables: close index_reader in has_partition_key
the "keyspace" argument of the "ring" command is optional. but before
this change, we considered it a mandatory option. it was wrong.
so, in this change, we make it optional, and print out the warning
message if the keyspace is not specified.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17472
* tighten the param check for toppartitions
* add an extra empty line inbetween reports
Closesscylladb/scylladb#17486
* github.com:scylladb/scylladb:
tools/scylla-nodetool: add an extra empty line inbetween reports
tools/scylla-nodetool: tighten the param check for toppartitions
RPC calls lose information about the type of returned exception.
Thus, if a table is dropped on receiver node, but it still exists
on a sender node and sender node streams the table's data, then
the whole operation fails.
To prevent that, add a method which synchronizes schema and then
checks, if the exception was caused by table drop. If so,
the exception is swallowed.
Use the method in streaming and repair to continue them when
the table is dropped in the meantime.
Fixes: #17028.
Fixes: #15370.
Fixes: #15598.
Closesscylladb/scylladb#17231
* github.com:scylladb/scylladb:
repair: handle no_such_column_family from remote node gracefully
test: test drop table on receiver side during streaming
streaming: fix indentation
streaming: handle no_such_column_family from remote node gracefully
repair: add methods to skip dropped table
would be more helpful if the matched could print out the unmatched
output on test failure. so, in this change, both stdout and stderr
are printed if they fail to match with the expected error.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17489
simpler this way.
Closesscylladb/scylladb#17437
* github.com:scylladb/scylladb:
tools/scylla-nodetool: use {yaml,json}_writers in compactionhistory_operation
tools/scylla-nodetool: add {json,yaml}_writer
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `sstables::deletion_time`,
drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `sstables::indexable_element`,
drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `sstables::sstable_state`,
drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `sstables::bound_kind_m`,
drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, `toppartitions` does not print an empty line
after an empty sampling warning message. but
dtest/toppartitions_test.py actually split sampling reports with
two newlines, so let's appease it. the output also looks better
this way, as the samplings for READS and WRITES are always visually
separated with an empty line.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
the test cases of `test_any_of_required_parameters_is_missing`
considers that we should either pass all positional argument or
pass none of them, otherwise nodetool should fail. but `scylla nodetool`
supported partial positional argument.
to be more consistent with the expected behavior, in this change,
we enforce the sanity check so that we only accept either all
positional args or none of them. the corresponding test is added.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* managed_bytes
* managed_bytes_view
* managed_bytes_opt
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `occupancy_stats`, and
drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for the classes derived from `tree_test_key_base`
(this change was extracted from a larger change at #15599)
Refs #13245
after dropping the operator<< for vector, we would not able to
use BOOST_REQUIRE_EQUAL to compare vector<>. to be prepared for this,
less defined the printer for Boost.test
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
the operator<< for `cql3::expr::test_utils::mutation_column_value` is
preserved, as it used by test/lib/expr_test_utils.cc, which prints
std::map<sstring, cql3::expr::test_utils::mutation_column_value> using
the homebrew generic formatter for std::map<>. and the formatter uses
operator<< for printing the elements in map.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* scheduling_latency_measurer
* perf_result
and drop their operator<<:s
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for `alternator::parsed::path`,
and drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17458
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* row_tombstone
* row_marker
* deletable_row::printer
* row::printer
* clustering_row::printer
* static_row::printer
* partition_start
* partition_end
* mutation_fragment::printer
and drop their operator<<:s
Refs #13245Closesscylladb/scylladb#17461
* github.com:scylladb/scylladb:
mutation: add fmt::formatter for clustering_row and friends
mutation: add fmt::formatter for row_tombstone and friends
If index_reader isn't closed before it is destroyed, then ongoing
sstables reads won't be awaited and assertion will be triggered.
Close index_reader in has_partition_key before destroying it.
Our interval template started life as `range`, and was supported wrapping to follow Cassandra's convention of wrapping around the maximum token.
We later recognized that an interval type should usually be non-wrapping and split it into wrapping_range and nonwrapping_range, with `range` aliasing wrapping_range to preserve compatibility.
Even later, we realized the name was already taken by C++ ranges and so renamed it to `interval`. Given that intervals are usually non-wrapping, the default `interval` type is non-wrapping.
We can now simplify it further, recognizing that everyone assumes that an interval is non-wrapping and so doesn't need the nonwrapping_interval_designation. We just rename nonwrapping_interval to `interval` and remove the type alias.
Closesscylladb/scylladb#17455
* github.com:scylladb/scylladb:
interval: rename nonwrapping_interval to interval
interval: rename interval_test to wrapping_interval_test
in af2553e8, we added formatters for cdc::image_mode and
cdc::delta_mode. but in that change, we failed to qualify `string_view`
with `std::` prefix. even it compiles, it depends on a `using
std::string_view` or a more error-prone `using namespace std`.
neither of which shold be relied on. so, in this change, we
add the `std::` prefix to `string_view`.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17459
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* `streaming::stream_request`,
* `stream_session_state`
and drop their operator<<:s
Refs #13245Closesscylladb/scylladb#17464
* github.com:scylladb/scylladb:
streaming: add fmt::formatter for streaming::stream_request
streaming: add fmt::formatter for stream_session_state
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* `sstables::compaction_type`
* `sstables::compaction_type_options::scrub::mode`
* `sstables::compaction_type_options::scrub::quarantine_mode``
and drop their operator<<:s.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17441
cluster_status_table virtual table have a status field for each node. In
gossiper mode the status is taken from the gossiper, but with raft the
states are different and are stored in the topology state machine. The
series fixes the code to check current mode and take the status from
correct place.
Refs scylladb/scylladb#16984
* 'gleb/cluster_status_table-v1' of github.com:scylladb/scylla-dev:
gossiper: remove unused REMOVAL_COORDINATOR state
virtual_tables: take node state from raft for cluster_status_table table if topology over raft is enabled
virtual_tables: create result for cluster_status_table read on shard 0
When we create a CDC generation and ring-delay is non-zero, the
timestamp of the new generation is in the future. Hence, we can
have multiple generations that can be written to. However, if we
add a new node to the cluster with the Raft-based topology, it
receives only the last committed generation. So, this node will
be rejecting writes considered correct by the other nodes until
the last committed generation starts operating.
In scylladb/scylladb#17134, we have allowed sending writes to the
previous CDC generations. So, the situation became even more
complicated. This PR adjusts the Raft-based topology
to ensure all required generations are loaded into memory and their
data isn't cleared too early.
To load all required generations into memory, we replace
`current_cdc_generation_{uuid, timestamp}` with the set containing
IDs of all committed generations - `committed_cdc_generations`.
To ensure this set doesn't grow endlessly, we remove an entry from
this set together with the data in CDC_GENERATIONS_V3.
Currently, we may clear a CDC generation's data from
CDC_GENERATIONS_V3 if it is not the last committed generation
and it is at least 24 hours old (according to the topology
coordinator's clock). However, after allowing writes to the
previous CDC generations, this condition became incorrect. We
might clear data of a generation that could still be written to.
The new solution introduced in this PR is to clear data of the
generations that finished operating more than 24 hours ago.
Apart from the changes mentioned above, this PR hardens
`test_cdc_generation_clearing.py`.
Fixesscylladb/scylladb#16916Fixesscylladb/scylladb#17184Fixesscylladb/scylladb#17288Closesscylladb/scylladb#17374
* github.com:scylladb/scylladb:
test: harden test_cdc_generation_clearing
test: test clean-up of committed_cdc_generations
raft topology: clean committed_cdc_generations
raft topology: clean only obsolete CDC generations' data
storage_service: topology_state_load: load all committed CDC generations
system_keyspace: load_topology_state: fix indentation
raft topology: store committed CDC generations' IDs in the topology
removenode --force is an unsafe operation and does not even make sense with
topology over raft. This patch disables it if raft is enabled and prints
a deprecation note otherwise. We already have a PR to remove it
(https://github.com/scylladb/scylladb/pull/15834), but it was decided
there that a deprecation period is needed for legacy use case.
Fixes: scylladb/scylladb#16293
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
in this change, we define formatters for
* clustering_row::printer
* static_row::printer
* partition_start
* partition_end
* mutation_fragment::printer
and drop their operator<<:s
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
when '\' does not start an escape sequence, Python complains at seeing
it. but it continues anyway by considering '\' as a separate char.
but the warning message is still annoying:
```
scylla-gdb.py: 2417: SyntaxWarning: invalid escape sequence '\-'
branches = (r" |-- ", " \-- ")
```
when sourcing this script.
so, let's mark these strings as raw strings.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#17466
