currently, our homebrew formatter formats `std::map` like
```
{{k1, v1}, {k2, v2}}
```
while {fmt} formats a map like:
```
{k1: v1, k2: v2}
```
and if the type of key/value is string, {fmt} quotes it, so a
compaction strategy option is formatted like
```
{"max_threshold": "1"}
```
before switching the formatter to the ones supported by {fmt},
let's update the test to match with the new format. this should
reduce the overhead of reviewing the change of switching the
formatter. we can revert this change, and use a simpler approach
after the change of formatter lands.
Closesscylladb/scylladb#18058
* github.com:scylladb/scylladb:
test/cql-pytest: match error message formated using {fmt}
test/cql-pytest: extract scylla_error() for not allowed options test
Test.py uses `ring_delay_ms = 0` by default. CDC creates generation's timestamp by adding `ring_delay_ms` to it.
In this test, nodes are learning about new generations (introduced by upgrade procedure and then by node bootstrap) concurrently with doing writes that should go to these generations.
Because of `ring_delay_ms = 0', the generation could have been committed when it should have already been in use.
This can be seen in the following logs from a node:
```
ERROR 2024-03-22 12:29:55,431 [shard 0:strm] cdc - just learned about a CDC generation newer than the one used the last time streams were retrieved. This generation, or some newer one, should have been used instead (new generation's timestamp: 2024/03/22 12:29:54, last time streams were retrieved: 2024/03/22 12:29:55). The new generation probably arrived too late due to a network partition and we've made a write using the wrong set streams.
```
Creating writes during such a generation can result in assigning them a wrong generation or a failure. Failure may occur if it hits short time window when `generation_service::handle_cdc_generation(cdc::generation_id_v2)` has executed
`svc._cdc_metadata.prepare(...)` but`_cdc_metadata.insert(...)` has not yet been executed. With a nonzero ring_delay_ms it's not a problem, because during this time window, the generation should not be in use.
Write can fail with the following response from a node:
```
cdc: attempted to get a stream from a generation that we know about, but weren't able to retrieve (generation timestamp: 2024/03/22 12:29:54, write timestamp: 2024/03/22 12:29:55). Make sure that the replicas which contain this generation's data are alive and reachable from this node.
```
Set ring_delay_ms to 15000 for the debug mode and 5000 in other modes. Wait for the last generation to be in use and sleep one second to make sure there are writes to the CDC table in this generation.
Fixesscylladb/scylladb#17977
Reapply b4144d14c6.
Closesscylladb/scylladb#17998
* github.com:scylladb/scylladb:
test.py: test_topology_upgrade_basic: make ring_delay_ms nonzero
Reapply "test.py: adjust the test for topology upgrade to write to and read from CDC tables"
currently, our homebrew formatter formats `std::map` like
{{k1, v1}, {k2, v2}}
while {fmt} formats a map like:
{k1: v1, k2: v2}
and if the type of key/value is string, {fmt} quotes it, so a
compaction strategy option is formatted like
{"max_threshold": "1"}
before switching the formatter to the ones supported by {fmt},
let's update the test to match with the new format. this should
reduce the overhead of reviewing the change of switching the
formatter. we can revert this change, and use a simpler approach
after the change of formatter lands.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
currently, our homebrew formatter formats `std::map` like
{{k1, v1}, {k2, v2}}
while {fmt} formats a map like:
{k1: v1, k2: v2}
and if the type of key/value is string, {fmt} quotes it, so a
compaction strategy option is formatted like
{"max_threshold": "1"}
as we are switching to the formatters provided by {fmt}, would be
better to support its convention directly.
so, in this change, to prepare the change, before migrating to
{fmt}, let's refactor the test to support both formats by
extracting a helper to format the error message, so that we can
change it to emit both formats.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Test.py uses `ring_delay_ms = 0` by default. CDC creates generation's timestamp
by adding `ring_delay_ms` to it.
In this test, nodes are learning about new generations (introduced by upgrade
procedure and then by node bootstrap) concurrently with doing writes that
should go to these generations.
Because of `ring_delay_ms = 0', the generation could have been committed when
it should have already been in use.
This can be seen in the following logs from a node:
```
ERROR 2024-03-22 12:29:55,431 [shard 0:strm] cdc - just learned about a CDC generation newer than the one used the last time streams were retrieved. This generation, or some newer one, should have been used instead (new generation's timestamp: 2024/03/22 12:29:54, last time streams were retrieved: 2024/03/22 12:29:55). The new generation probably arrived too late due to a network partition and we've made a write using the wrong set streams.
```
Creating writes during such a generation can result in assigning them a wrong
generation or a failure. Failure may occur if it hits short time window when
`generation_service::handle_cdc_generation(cdc::generation_id_v2)` has executed
`svc._cdc_metadata.prepare(...)` but`_cdc_metadata.insert(...)` has not yet
been executed. With a nonzero ring_delay_ms it's not a problem, because during
this time window, the generation should not be in use.
Write can fail with the following response from a node:
```
cdc: attempted to get a stream from a generation that we know about, but weren't able to retrieve (generation timestamp: 2024/03/22 12:29:54, write timestamp: 2024/03/22 12:29:55). Make sure that the replicas which contain this generation's data are alive and reachable from this node.
```
Set ring_delay_ms to 15000 for the debug mode and 5000 in other modes.
Wait for the last generation to be in use and sleep one second to make sure
there are writes to the CDC table in this generation.
Fixes#17977
this series includes test related changes to enable us to drop `FMT_DEPRECATED_OSTREAM` deprecated in {fmt} v10.
Refs #13245Closesscylladb/scylladb#18054
* github.com:scylladb/scylladb:
test: unit: add fmt::formatter for test_data in tests
test/lib: do not print with fmt::to_string()
test/boost: print runtime_error using e.what()
* 'gleb/raft_snapshot_rpc-v3' of github.com:scylladb/scylla-dev:
raft topology: drop RAFT_PULL_TOPOLOGY_SNAPSHOT RPC
Use correct limit for raft commands throughout the code.
When repairing multiple keyspaces, bail out on the first failed keyspace repair, instead of continuing and reporting all failures at the end. This is what Origin does as well.
To be able to test this, a bit of refactoring was needed, to be able to assert that `scylla-nodetool` doesn't make repair requests, beyond the expected ones.
Refs: https://github.com/scylladb/scylla-cluster-tests/issues/7226Closesscylladb/scylladb#17678
* github.com:scylladb/scylladb:
tools/scylla-nodetool: repair: abort on first failed repair
test/nodetool: nodetool(): add check_return_code param
test/nodetool: nodetool(): return res object instead of just stdout
test/nodetool: count unexpected requests
This series provides a reallocate_tablets function, that's initially called by allocate_tablets_for_new_table.
The new allocation implementation is independent of vnodes/token ownership.
Rather than using the natural_endpoints_tracker, it implements its own tracking
based on dc/rack load (== number of replicas in rack), with the additional benefit
that tablet allocation will balance the allocation across racks, using a heap structure,
similar to the one we use to balance tablet allocation across shards in each node.
reallocate_tablets may also be called with an optional parameter pointing the the current tablet_map.
In this case the function either allocates more tablet replicas in datacenters for which the replication factor was increased,
or it will deallocate tablet replicas from datacenters for which replication factor was decreased.
The NetworkTopologyStrategy_tablets_test unit test was extended to cover replication factor changes.
Closesscylladb/scylladb#17846
* github.com:scylladb/scylladb:
network_topology_strategy: reallocate_tablets: consider new_racks before existing racks
network_topology_startegy_test: add NetworkTopologyStrategy_tablet_allocation_balancing_test
network_topology_strategy: reallocate_tablets: support deallocation via rf change
network_topology_startegy_test: tablets_test: randomize cases
network_topology_strategy: allocate_tablets_for_new_table: do not rely on token ownership
network_topology_startegy_test: add NetworkTopologyStrategy_tablets_negative_test
network_topology_strategy_test: endpoints_check: use particular BOOST_CHECK_* functions
network_topology_strategy_test: endpoints_check: verify that replicas are placed on unique nodes
network_topology_strategy_test: endpoints_check: strictly check rf for tablets
network_topology_strategy_test: full_ring_check for tablets: drop unused options param
our homebrew formatter for std::vector<string> formats like
```
{hello, world}
```
while {fmt}'s formatter for sequence-like container formats like
```
["hello", "world"]
```
since we are moving to {fmt} formatters. and in this context,
quoting the verbatim text makes more sense to user. let's
support the format used by {fmt} as well.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18057
Raft uses schema commitlog, so all its limits should be derived from
this commitlog segment size, but many places used regular commitlog size
to calculate the limits and did not do what they really suppose to be
doing.
Before this series, Alternator's Query and Scan operations convert an
entire result page to JSON without yielding. For a page of maximum
size (1MB) and tiny rows, this can cause a significant stall - the
test included in this PR reported stalls of 14-26ms on my laptop.
The problem is the describe_items() function, which does this conversion
immediately, without yielding. This patch changes this function to
return a future, and use a new result_set::visit_gently() method
that does what visit() does, but with yields when needed.
This PR improves #17995, but does not completely fix is as the stalls in the
are not completely eliminated. But on my laptop it usually reduces the stalls
to around 5ms. It appears that the remaining stalls some from other places
not fixed in this PR, such as perhaps query_page::handle_result(), and will need
to be fixed by additional patches.
Closesscylladb/scylladb#18036
* github.com:scylladb/scylladb:
alternator: reduce stall for Query and Scan with large pages
result_set: introduce visit_gently()
alternator: coroutinize do_query() function
Memtables are fickle, they can be flushed when there is memory pressure,
if there is too much commitlog or if there is too much data in them. The
tests in test_select_from_mutation_fragments.py currently assume data
written is in the memtable. This is tru most of the time but we have
seen some odd test failures that couldn't be understood. To make the
tests more robust, flush the data to the disk and read it from the
sstables. This means that some range scans need to filter to read from
just a single mutation source, but this does not influence the tests.
Also fix a use-after-return found when modifying the tests.
This PR tentatively fixes the below issues, based on our best guesses on why they failed (each was seen just once):
Fixes: scylladb/scylladb#16795Fixes: scylladb/scylladb#17031Closesscylladb/scylladb#17562
* github.com:scylladb/scylladb:
test/cql-pytest: test_select_from_mutation_fragments.py: move away from memtables
cql3: select_statement: mutation_fragments_select_statement: fix use-after-return
This change adds the missing Cassandra compaction option unchecked_tombstone_compaction.
Setting this option to true causes the compaction to ignore tombstone_threshold, and decide whether to do a compaction only based on the value of tombstone_compaction_interval
Fixes#1487Closesscylladb/scylladb#17976
* github.com:scylladb/scylladb:
removed forward declaration of resharding_descriptor
compaction options and troubleshooting docs
cql-pytest/test_compaction_strategy_validation.py
test/boost/sstable_compaction_test.cc
compaction: implement unchecked_tombstone_compaction
Calling `_next_row.get_iterator_in_latest()` is illegal when `_next_row` is not
pointing at a row. In particular, the iterator returned by such call might be
dangling.
We have observed this to cause a use-after-free in the field, when a reverse
read called `maybe_add_to_cache` after `_latest_it` was left dangling after
a dead row removal in `copy_from_cache_to_buffer`.
To fix this, we should ensure that we only call `_next_row.get_iterator_in_latest`
is pointing at a row.
Only the occurrences of this problem in `maybe_add_to_cache` are truly dangerous.
As far as I can see, other occurrences can't break anything as of now.
But we apply fixes to them anyway.
Closesscylladb/scylladb#18046
Fixesscylladb/scylladb#17893
* 'gleb/initial-token-v1' of github.com:scylladb/scylla-dev:
dht: drop unused parameter from get_random_bootstrap_tokens() function
test: add test for initial_token parameter
topology coordinator: use provided initial_token parameter to choose bootstrap tokens
topology cooordinator: propagate initial_token option to the coordinator
this change is created in same spirit of d1c35f943d.
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 test_data in
radix_tree_stress_test.cc, and drop its operator<<.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
we should not format a variable unless we want to print it. in this
case, we format `first_row` using `fmt::to_string()` to a string,
and then insert the string to another string, despite that this is
in a cold path, this is still a anti pattern -- both convoluted,
and not performant.
so let's just pass `first_row` to `format()`.
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. but fortunately, fmt v10 brings the builtin
formatter for classes derived from `std::exception`. but before
switching to {fmt} v10, and after dropping `FMT_DEPRECATED_OSTREAM`
macro, we need to print out `std::runtime_error`. so far, we don't
have a shared place for formatter for `std::runtime_error`. so we
are addressing the needs on a case-by-case basis.
in this change, we just print it using `e.what()`. it's behavior
is identical to what we have now.
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Add support for deallocating tablet replicas when the
datacenter replication factor is decreased.
We deallocate replicas back-to-front order to maintain
replica pairing between the base table and
its materialized views.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Instead of deterministically testing a very small set of cases,
randomize the the shard_count per node, the cluster topology
and the NetworkTopologyStrategy options.
The next patch will extend the test to also test
`reallocate_tablets` with randomized options.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
When repairing multiple keyspaces, bail out on the first failed keyspace
repair, instead of continuing and reporting all failures at the end.
This is what Origin does as well.
Test that we attempting to allocate tablets
throws an error when there are not enough nodes
for the configured replication factor.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Using e.g. `BOOST_CHECK_EQUAL(endpoints.size(), total_rf)`
rather than `BOOST_CHECK(endpoints.size() == total_rf)`
prints a more detailed error message that includes the
runtime valies, if it fails.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
With tablet we want to verify that the number of
replicas allocated per tablet per dc exactly matches
the replication strategy per-dc replication factor options.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
When set to false, the returncode is not checked, this is left to the
caller. This in turn allows for checking the expected and unexpected
requests which is not checked when the nodetool process fails.
This is used by utils._do_check_nodetool_fails_with(), so that expected
and unexpected requests are checked even for failed invocations.
Some test need adjustment to the stricter checks.
We currently check at the end of each test, that all expected requests
set by the test were consumed. This patch adds a mechanism to count
unexpected requests -- requests which didn't match any of the expected
ones set by the test. This can be used to asser that nodetool didn't
make any request to the server, beyond what the test expected it to do.
Before this patch, requests like this would only be noticed by the test,
if the response of 404/500 caused nodetool to fail, which is not always
the case.
The CDC feature is not supported on a table that uses tablets
(Refs https://github.com/scylladb/scylladb/issues/16317), so if a user creates a keyspace with tablets enabled
they may be surprised later (perhaps much later) when they try to enable
CDC on the table and can't.
The LWT feature always had issue Refs https://github.com/scylladb/scylladb/issues/5251, but it has become potentially
more common with tablets.
So it was proposed that as long as we have missing features (like CDC or
LWT), every time a keyspace is created with tablets it should output a
warning (a bona-fide CQL warning, not a log message) that some features
are missing, and if you need them you should consider re-creating the
keyspace without tablets.
This PR does this.
The warning text which will be produced is the following (obviously, it can
be improved later, as we perhaps find more missing features):
> "Tables in this keyspace will be replicated using tablets, and will
> not support the CDC feature (issue https://github.com/scylladb/scylladb/issues/16317) and LWT may suffer from
> issue https://github.com/scylladb/scylladb/issues/5251 more often. If you want to use CDC or LWT, please drop
> this keyspace and re-create it without tablets, by adding AND TABLETS
> = {'enabled': false} to the CREATE KEYSPACE statement."
This PR also includes a test - that checks that this warning is is
indeed generated when a keyspace is created with tablets (either by default
or explicitly), and not generated if the keyspace is created without
tablets. It also fixes existing tests which didn't like the new warning.
Fixes https://github.com/scylladb/scylladb/issues/16807Closesscylladb/scylladb#17318
* github.com:scylladb/scylladb:
tablets: add warning on CREATE KEYSPACE
test/cql-pytest: fix guadrail tests to not be sensitive to more warnings
Before this patch, Alternator's Query and Scan operations convert an
entire result page to JSON without yielding. For a page of maximum
size (1MB) and tiny rows, this can cause a significant stall - the
test included in this patch reported stalls of 14-26ms on my laptop.
The problem is the describe_items() function, which does this conversion
immediately, without yielding. This patch changes this function to
return a future, and use the result_set::visit_gently() method
instead of visit() that yields when needed.
This patch does not completely eliminate stalls in the test, but
on my laptop usually reduces them to around 5ms. It appears that
the remaining stalls some from other places not fixed in this PR,
such as perhaps query_page::handle_result(), and will need to be
fixed by additional patches.
The test included in this patch is useful for manually reproducing
the stall, but not useful as a regression test: It is slow (requiring
a couple of seconds to set up the large partition) and doesn't
check anything, and can't even report the stall without modifying the
test runner. So the test is skipped by default (using the "veryslow"
marker) and can be enabled and run manually by developers who want
to continue working on #17995.
Refs #17995.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
feature_service.hh is a high-level header that integrates much
of the system functionality, so including it in lower-level headers
causes unnecessary rebuilds. Specifically, when retiring features.
Fix by removing feature_service.hh from headers, and supply forward
declarations and includes in .cc where needed.
Closesscylladb/scylladb#18005
These commands manage to avoid detection because they are not documented on https://opensource.docs.scylladb.com/stable/operating-scylla/nodetool.html.
They were discovered when running dtests, with ccm tuned to use the native nodetool directly. See https://github.com/scylladb/scylla-ccm/pull/565.
The commands come with tests, which pass with both the native and Java nodetools. I also checked that the relevant dtests pass with the native implementation.
Closesscylladb/scylladb#17979
* github.com:scylladb/scylladb:
tools/scylla-nodetool: implement the sstableinfo command
tools/scylla-nodetool: implement the getsstables command
tools/scylla-nodetool: move get_ks_cfs() to the top of the file
test/nodetool: rest_api_mock.py: add expected_requests context manager
before this change, we rely on the default-generated fmt::formatter
created from operator<<, but fmt v10 dropped the default-generated
formatter.
also, it's impossible to partial specialize a nested type of a
template class, we cannot specialize the `fmt::formatter` for
`stop_crash<M>::result_type`, as a workaround, a new type is
added.
in this change,
* define a new type named `stop_crash_result`
* add fmt::formatter for `stop_crash_result`
* define stop_crash::result_type as an alias of `stop_crash_result`
Refs #13245
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18018
`UUID::to_sstring()` relies on `FMT_DEPRECATED_OSTREAM` to generated `fmt::formatter` for `UUID`, and this feature is deprecated in {fmt} v9, and dropped in {fmt} v10.
in this series, all callers of `UUID::to_sstring()` are switched to `fmt::to_string()`, and this function is dropped.
Closesscylladb/scylladb#18020
* github.com:scylladb/scylladb:
utils: UUID: drop UUID::to_sstring()
treewide: use fmt::to_string() to transform a UUID to std::string
There are skip_in_<mode> lists in suite yaml that tells test.py not to run the test from it. This PR sanitizes these lists in two ways.
First, to skip pytests the skip-decorators are much more convenient, e.g. because they show the reason why the test is skipped.
Also, if a test wants to be opt-in-ed for some mode only, it's opt-out-ed in all other lists instead. There's run_in_<mode> list in suite for that.
Closesscylladb/scylladb#17964
* github.com:scylladb/scylladb:
test: Do not duplicate test name in several skip-lists
test: Mark tests with skip_mode instead of suite skip-list
without `FMT_DEPRECATED_OSTREAM` macro, `UUID::to_sstring()` is
implemented using its `fmt::formatter`, which is not available
at the end of this header file where `UUID` is defined. at this moment,
we still use `FMT_DEPRECATED_OSTREAM` and {fmt} v9, so we can
still use `UUID::to_sstring()`, but in {fmt} v10, we cannot.
so, in this change, we change all callers of `UUID::to_sstring()`
to `fmt::to_string()`, so that we don't depend on
`FMT_DEPRECATED_OSTREAM` and {fmt} v9 anymore.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
So tests and fixtures can use `with expected_requests():` and have
cleanup be taken care for them. I just discovered that some tests do not
clean up after themselves and when running all tests in a certain order,
this causes unrelated tests to fail.
Fix by using the context everywhere, getting guaranteed cleanup after
each test.
The test creates ut4 with a lot of fields,
this may take a while in debug builds,
to avoid raft operation timeout set the threshold
to some big value.
The error injector is disabled in release builds,
so this settings won't be applied to them.
This shouldn't be a problem since release builds
are fast enough, even on arm.
Fixesscylladb/scylladb#17987Closesscylladb/scylladb#17997
Currently, the tests in test/cql-pytest can be run against both ScyllaDB and Cassandra.
Running the test for either will first output the test results, and subsequently
print the stdout output of the process under test. Using the command line
option --omit-scylla-output it is possible to disable this print for Scylla,
but it is not possible for tests run against Cassandra.
This change adds the option to suppress output for Cassandra tests, too. By default,
the stdout of the Cassandra run will still be printed after the test results, but
this can now be disabled with --omit-scylla-output
Closesscylladb/scylladb#17996
