This patch adds a requirement for the "DROP" permission on a table to
run a DeleteTable on it.
Moreover, when a table and its views are deleted, any special permissions
previously GRANTed on this table are removed. This is necessary because
if a role creates a table it is automatically granted permissions on this
table (this is known as "auto-grant" - see the CreateTable patch for
details). If this role deletes this table and later a second role creates
a table with the same name, we don't want the first role to have
permissions on this new table.
Tests for permission enforcements and revocation on delete are also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "MODIFY" permission on a table to
run a UpdateItem on it.
Only the MODIFY permission is required, even if the operation may also
read the old value of the item, such as a read-modify-write operation
or even using ReturnValues='ALL_OLD'.
A test is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "MODIFY" permission on a table to
run a DeleteItem on it.
Only the MODIFY permission is required, even if the operation may also
read the old value of the item (using ReturnValues='ALL_OLD').
A test is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "MODIFY" permission on a table to
run a PutItem on it.
Only the MODIFY permission is required, even if the operation may also
read the old value of the item (using ReturnValues='ALL_OLD').
A test is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
In this patch, we begin to add role-based access control (RBAC)
enforement to Alternator - in this patch only to GetItem.
After the preparation of client_state correctly in the previous patch,
the permission check itself in the get_item() function is very simple.
The bigger part of this patch is a full functional test in
test/alternator/test_cql_rbac.py. The test is quite self-explanatory
and heavily commented. Basically we check that a new role cannot
read with GetItem a pre-existing table, and we can add that ability
by GRANTing (in CQL) the new role the ability to SELECT the table,
the keyspace, all keyspaces, or add that ability to some other role
that this role inherits.
In the following patches, we will add role-based access control to
the Alternator operations, but the functional tests will be shorter -
we don't need to check the role inheritence, "all keyspaces" feature,
and so on, for every operation separately since they all use the
same underlying checking functions which handles these role inheritence
issues in exactly the same way.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds unit tests to verify the correctness of the newly
introduced histogram metrics for get and write batch operation
latencies.
The test uses the existing latency test with the added metrics.
Signed-off-by: Amnon Heiman <amnon@scylladb.com>
before this change, we assume user runs nodetool tests right under the root source directory. if user runs them under `test/nodetool`, the suppression rules are not applied. as the path is incorrect in that case.
after this change, the supression rules' path is deduced from the top src directory. so we can now run the nodetool test under `test/nodetool` .
---
no need to backport, this change improves developer's experience.
Closesscylladb/scylladb#20119
* github.com:scylladb/scylladb:
test/nodetool: deduce subpression path from top srcdir
test/nodetool: deduce path from top srcdir
The lock_table() method needs database, ks and cf to find the table on
all shards. The same can be achieved with the help of global_table_ptr
thing that all the core callers already have at hand.
There's a test that doesn't have global table, but it can get one.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#20139
Typically the sstable_directory is constructed out of a table object.
Some code, namely tests and schema-loader, don't have table at hand and
construct directory out of schema, sharder, path-to-sstables, etc. This
code doesn't work with any storage options other than local ones, so
there's no need (yet) to carry this argument over.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#20138
before this change, we look up for the mode using the command line
option as the key, but that's incorrect if the command line option does
not match with any of the known names. in that case, `test_mode` just
create another pair of <sstring, test_modes>, and return the second
component of this pair. and the second component is not what we expect.
we should have thrown an exception.
in this change
* the test_mode map is marked const.
* the overloads for parsing / formatting the `test_modes` type are
added, so that boost::program_options can parse and format it.
after this change, we print more user friendly error, like
```
/scylla perf-sstable --mode index-foo
error: the argument ('index-foo') for option '--mode' is invalid
Try --help.
```
instead of a bunch of output which is printed as if we passes the correct option as the argument of the `--mode` option.
---
it's an improvement of developer experience, hence no need to backport.
Closesscylladb/scylladb#20140
* github.com:scylladb/scylladb:
test/perf/perf_sstable: use switch-case when appropriate
test/perf/perf_sstables: use test_modes as the type of its option
This change fixes#17237, fixes#5361 and fixes#5362 by passing the limit value down the call chain in cql3. A test is also added.
fixes#17237fixes#5361fixes#5362
The regression happened in 5.4 as we changed the way GROUP BY is processed in 432cb02 - to force aggregation when it is used. The LIMIT value was not passed to aggregations and thus we failed to adhere to it.
W want to backport this fix to 5.4 and 6.0 to have continuous correct results for the test case from #17237
This patch consists of 4 commits:
- fa4225ea0fac2057b7a9976f57dc06bcbd900cd4 - cql3: respect the user-defined page size in aggregate queries - a precondition for this patch to be implementable
- 8fbe69e74dca16ed8832d9a90489ca47ba271d0b - cql3/select_statement: simplify the get_limit function - the `do_get_limit()` function did a lot of legwork that should not be associated with it. This change makes it trivial and makes its callers do additional checks (for unset guards, or for an aggregate query)
- 162828194a2b88c22fbee335894ff045dcc943c9 - cql3: process LIMIT for GROUP BY queries - pass the limit value down the chain and make use of it. This is the actual fix to #17237
- b3dc6de6d6cda8f5c09b01463bb52f827a6a00b4 - test/cql-pytest: Add test for GROUP BY queries with LIMIT - tests
Closesscylladb/scylladb#18842
* github.com:scylladb/scylladb:
test/cql-pytest: Add test for GROUP BY queries with LIMIT
cql3: process LIMIT for GROUP BY queries
cql3/select_statement: simplify the get_limit function
cql3: respect the user-defined page size in aggregate queries
Drop half-reversed (legacy) format of query::partition_slice.
The select query builds a fully reversed (native) slice for reversed queries and use it together with a reversed
schema to construct query::read_command that is further propagated to the database.
A cluster feature is added to support nodes that still operate on half-reversed slices. When the feature is turned off:
- query::read_command is transformed (to have table schema and half-reversed slices) before sending to other nodes
- query::read_command is transformed (to have query schema (reversed) and reversed slices) after receiving it from other nodes
- Similarly, mutations are transformed. They are reversed before being sent to other nodes or after receiving them from other nodes.
Additional manual tests were performed to test a mixed-node cluster:
1. 3-node cluster with one node upgraded: reverse read queries performed on an old node
2. 3-node cluster with one node upgraded: reverse read queries performed on a new node
3. 3-node cluster with one node upgraded and all its sstable files deleted to trigger repair: reverse read queries performed on an old node
4. 3-node cluster with one node upgraded and all its sstable files deleted to trigger repair: reverse read queries performed on a new node
All reverse read queries above consists of:
- single-partition reverse reads with no clustering key restrictions, with single column restrictions and multi column restrictions both with and without paging turned on
- multi-partition reverse reads with range restrictions with optional partition limit and partial ordering
The exact same tests were also performed on a fully upgraded cluster.
Fixes https://github.com/scylladb/scylladb/issues/12557Closesscylladb/scylladb#18864
* github.com:scylladb/scylladb:
mutation_partition: drop reverse parameter in compact_for_query
clustering_key_filter: unify get_ranges and get_native_ranges
streamed_mutation_freezer: drop the reverse parameter
reverse-reads.md: Drop legacy reverse format information
Fix comments refering to half-reversed (legacy) slices
select_statement::do_execute: Add tracing informaction
query::trim_clustering_row_ranges_to: require reversed schema for native reversed ranges
query-request: Drop half_reverse_slice as it is no longer used anywhere
readers: Use reversed schema and native reversed slices
database: accept reversed schema for reversed queries
storage_proxy: Support reverse queries in native format
query_pagers: Replace _schema with _query_schema
query_pagers: Support reverse queries in native format
select_statement: Execute reversed query in native format
storage_proxy::remote: Add support for mixed-node clusters
mutation_query: Add reversed function to reverse reconcilable_result
query-request: Add reversed function to reverse read_command
features: add native_reverse_queries
kl::reader::make_reader: Unify interface with mx::reader::make_reader
config: drop reversed_reads_auto_bypass_cache
config: drop enable_optimized_reversed_reads
If system_keyspace::stop() is called before system_keyspace::shutdown(),
it will never finish, because the uncleared shared pointers will keep
it alive indefinitely.
Currently this can happen if an exception is thrown before the construction
of the shutdown() defer. This patch moves the shutdown() call to immediately
before stop(). I see no reason why it should be elsewhere.
Fixesscylladb/scylla-enterprise#4380Closesscylladb/scylladb#20089
instead of using a chain of `if-else`, use switch-case instead,
it's visually easier to follow than `if`-`else` blocks. and since
we never need to handle the `else` case, the `throw` statement
is removed.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
before this change, we look up for the mode using the command line
option as the key, but that's incorrect if the command line option does
not match with any of the known names. in that case, `test_mode` just
create another pair of <sstring, test_modes>, and return the second
component of this pair. and the second component is not what we expect.
we should have thrown an exception.
in this change
* the test_mode map is marked const.
* the overloads for parsing / formatting the `test_modes` type are
added, so that boost::program_options can parse and format it.
after this change,
* we can print more user friendly error, like
```
/scylla perf-sstable --mode index-foo
error: the argument ('index-foo') for option '--mode' is invalid
Try --help.
```
instead of a bunch of output which is printed as if we passes the
correct option as the argument of the `--mode` option.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
ALTER tablets KS executes in 2 steps:
1. ALTER KS's cql handler forms a global topo req, and saves data required to execute this req,
2. global topo req is executed by topo coordinator, which reads data attached to the req.
The KS name is among the data attached to the req. There's a time window between these steps where a to-be-altered KS could have been DROPped, which results in topo coordinator forever trying to ALTER a non-existing KS. In order to avoid it, the code has been changed to first check if a to-be-altered KS exists, and if it's not the case, it doesn't perform any schema/tablets mutations, but just removes the global topo req from the coordinator's queue.
BTW. just adding this extra check resulted in broader than expected changes, which is due to the fact that the code is written badly and needs to be refactored - an effort that's already planned under #19126
(I suggest to disable displaying whitespace differences when reviewing this PR).
Fixes: scylladb/scylladb#19576Closesscylladb/scylladb#19666
* github.com:scylladb/scylladb:
tests: ensure ALTER tablets KS doesn't crash if KS doesn't exist
cql: refactor rf_change indentation
Prevent ALTERing non-existing KS with tablets
Using the error injection framework, we inject a sleep into the
processing path of ALTER tablets KS, so that the topology coordinator of
the leader node
sleeps after the rf_change event has been scheduled, but before it is
started to be executed. During that time the second node executes a DROP
KS statement, which is propagated to the leader node. Once leader node
wakes up and resumes processing of ALTER tablets KS, the KS won't exist
and the node cannot crash, which was the case before.
The reverse parameter is no longer used with native reverse reads.
The row ranges are provided in native reverse order together with
a reversed schema, thus the reverse parameter remain false all the
time and can be droped.
Simplify implementation and for clustering key ranges in native
reversed format, require a reversed table schema.
Trimming native reversed clustering key ranges requires a reversed
schema to be passed in. Thus, the reverse flag is no longer required
as it would always be set to false.
The reconcilable_result is built as it would be constructed for
forward read queries for tables with reversed order.
Mutations constructed for reversed queries are consumed forward.
Drop overloaded reversed functions that reverse read_command and
reconcilable_result directly and keep only those requiring smart
pointers. They are not used any more.
All lambdas passed to test_using_reusable_sst() conform to the prototype
void (test_env&, sstable_ptr)
All lambdas passed to test_using_reusable_sst_returning() conform to the
prototype
NON_VOID (test_env&, sstable_ptr)
The common parameter list of both prototypes can be expressed with the
concept
std::invocable<test_env&, sstable_ptr>
Once a "Func" template parameter (i.e., function type) satisfying this
concept is taken, then "Func"'s void or non-void return type can be
commonly expressed with
std::invoke_result_t<Func, test_env&, sstable_ptr>
In turn, test_env::do_with_async_returning<...> can be instantiated with
this return type, even if it happens to be "void".
([stmt.return] specifies, "[a] return statement with an operand of type
void shall be used only in a function that has a cv void return type",
meaning that
return func(env)
will do the right thing in the body of
test_env::do_with_async_returning<void>().)
Merge test_using_reusable_sst() and test_using_reusable_sst_returning()
into one. Preserve the function name from the former, and the
test_env::do_with_async_returning<...>() call from the latter.
Suggested-by: Avi Kivity <avi@scylladb.com>
Signed-off-by: Laszlo Ersek <laszlo.ersek@scylladb.com>
Closesscylladb/scylladb#20090
there are chances that developer launch `pytest` right under
`test/nodetool`, in that case current working directory is not
the root directory of the project, so the path to suppression rules
does not point to a file.
to cater the needs to run the test under `test/nodetool`, let's
use the path deduced from the top_srcdir.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Unit testing for the SSTable validation API happens in
`sstable_validate_test`. Currently, this test checks the API against
some invalid SSTables with out-of-order clustering rows and out-of-order
partitions. However, both are types of content-level corruption that do
not trigger `malformed_sstable_exception` errors.
Extend the test to cover cases of file-level corruption as well, i.e.,
cases that would raise a `malformed_sstable_exception`. Construct an
SSTable with an invalid checksum to trigger this.
This is part of the effort to improve scrub to handle all kinds of
corruption.
Fixesscylladb/scylladb#19057
Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
Closesscylladb/scylladb#20096
Currently, each change to tablet metadata triggers a full metadata reload from disk. This is very wasteful, especially if the metadata change affects only a single row in the `system.tablets` table. This is the case when the tablet load balancer triggers a migration, this will affect a single row in the table, but today will trigger a full reload.
We expect tablet count to potentially grow to thousands and beyond and the overhead of this full reload can become significant.
This PR makes tablet metadata reload partial, instead of reloading all metadata on topology or schema changes, reload only the partitions that are affected by the change. Copy the rest from the in-memory state.
This is done with two passes: first the change mutations are scanned and a hint is produced. This hint is then passed down to the reload code, which will use it to only reload parts (rows/partitions) of the metadata that has actually changed.
The performance difference between full reload and partial reload is quite drastic:
```
INFO 2024-07-25 05:06:27,347 [shard 0:stat] testlog - Tablet metadata reload:
full 616.39ms
partial 0.18ms
```
This was measured with the modified (by this PR) `perf_tablets`, which creates 100 tables, each with 2K tablets. The test was modified to change a single tablet, then do a full and partial reload respectively, measuring the time it takes for reach.
Fixes: #15294
New feature, no backport needed.
Closesscylladb/scylladb#15541
* github.com:scylladb/scylladb:
test/perf/perf_tablets: add tablet metadata reload perf measurement
test/boost/tablets_test: add test for partial tablet metadata updates
db/schema_tables: pass tablet hint to update_tablet_metadata()
service/storage_service: load_tablet_metadata(): add hint parameter
service/migration_listener: update_tablet_metadata(): add hint parameter
service/raft/group0_state_machine: provide tablet change hint on topology change
service/storage_service: topology_state_load(): allow providing change hint
replica/tablets: add update_tablet_metadata()
replica/tablets: fix indentation
replica/tablets: extract tablet_metadata builder logic
replica/tablets: add get_tablet_metadata_change_hint() and update_tablet_metadata_change_hint()
locator/tablets: add tablet_map::clear_tablet_transition_info()
locator/tablets: make tablet_metadata cheap to copy
mutation/canonical_mutation: add key()
Measure reload perf of full reload vs. partial reload, after changing a
single tablet.
While at it, modify the `--tablets-per-table` parameter, so that it has
a default parameter which works OOTB. The previous default was both too
large (causing oversized commitlog entry errors) and not a power of two.
The hint contains information related to what exactly changed, allowing
listeners to do partial updates, instead of reloading all metadata on
each notification.
Keep lw_shared_ptr<tablet_map> in the tablet map and use COW semantics.
To prevent accidental changes to shared tablet_map instances, all
modifications to a tablet_map have to go through a new
`mutate_tablet_map()` method, which implements the copy-modify-swap
idiom.
Fixes#19960
Write path for sstables/commitlog need to handle the fact that IO extensions can
generate errors, some of which should be considered retry-able, and some that should,
similar to system IO errors, cause the node to go into isolate mode.
One option would of course be for extensions to simply generate std::system_errors,
with system_category and appropriate codes. But this is probably a bad idea, since
it makes it more muddy at which level an error happened, as well as limits the
expressibility of the error.
This adds three distinct types (sharing base) distinguishing permission, availabilty
and configuration errors. These are treated akin to EACCESS, ENOENT and EINVAL in
disk error handler and memtable write loop.
Tests updated to use and verify behaviour.
Closesscylladb/scylladb#19961
If parent_info argument of compaction_manager::perform_compaction
is std::nullopt, then created compaction executor isn't tracked by task
manager. Currently, all compaction operations should by visible in task
manager.
Modify split methods to keep split executor in task manager. Get rid of
the option to bypass task manager.
Closesscylladb/scylladb#19995
* github.com:scylladb/scylladb:
compaction: replace optional<task_info> with task_info param
compaction: keep split executor in task manager
There are some bugs missed in task handler:
- wait_for_task does not wait until virtual tasks are done, but returns the status immediately;
- wait_for_task suffers from use after return;
- get_status_recursively does not set the kind of task essentials.
Fix the aforementioned.
Closesscylladb/scylladb#19930
* github.com:scylladb/scylladb:
test: add test to check that task handler is fixed
tasks: fix task handler
Remove xfail from all tests for #5361, as the issue is fixed.
Remove xfail from test_group_by_clustering_prefix_with_limit
It references #5362, but is fixed by #17237.
Refs #17237
This patch adds `suppress_features` error injection. It allows to revoke
support for some features and it can be used to simulate upgrade process
in test.py.
Features to suppress are passed as injection's value, separated by `;`.
Example: `PARALLELIZED_AGGREGATION;UDA_NATIVE_PARALLELIZED_AGGREGATION`
Fixesscylladb/scylladb#20034Closesscylladb/scylladb#20055
This patch makes all cql connections update theirs service level parameters automatically when:
- any service level is created or changed
- one role is granted to another
- any service level is attached to/detached from a role
First of all, the patch defines what a service level and an effective service level are 938aa10509. No new type of service levels are introduced, the commit only clarifies definitions and names what an effective service level is.
(Effective service level is created by merging all service levels which are attached to all roles granted to the user. It represents exact values of connection's parameters.)
Previously, to find an effective service level of a user, it required O(n) internal queries: O(n) queries to recursively find all granted roles (`standard_role_manager::query_granted()`) and a query for each role to get its service level (`standard_role_manager::get_attribute()`, which sums to O(n) queries).
Because we want to reload SL parameters for all opened cql connections, we don't want to do O(n) queries for every connection, every time we create or change any service level/grant one role to another/attach or detach a service level to/from a role.
To speed it up, the patch adds another layer of service level controller cache, which stored `role_name -> effective_service_level` mapping. This way finding a effective service level for a role is only a lookup to a map.
Building the new cache requires only 2 queries: one to obtain all role hierarchy one to get all roles' service level.
Fixesscylladb/scylladb#12923Closesscylladb/scylladb#19085
* github.com:scylladb/scylladb:
test/auth_cluster/test_raft_service_levels: add test for automatic connection update
api/cql_server_test: add CQL server testing API
transport/cql_server: subscribe to sl effective cache reloaded
transport/controller: coroutinize `subscribe_server` and `unsubscribe_server`
transport/cql_server: add method to update service level params on all connections
generic_server: use async function in `for_each_gently()`
service/qos/sl_controller: use effective service levels cache
service/qos/service_level_controller: notify subscribers on effective cache reloaded
service/raft/group0_state_machine: update effective service levels cache
service/topology_coordinator: migrate service levels before auth
service/qos/service_level_controller: effective service levels cache
utils/sorting: allow to pass any container as verticies
service/qos/service_level_controller: replace shard check to assert
service/qos: define effective service level
service/qos/qos_common: use const reference in `init_effective_names()`
service/qos/service_level_controller: remove unused field
auth: return map of directly granted roles
test/auth/test_auth_v2_migration: create sl1 in the test
We have two mechanism to give visibility into reads having to process many tombstones:
* a warning in the logs, triggered if a read processed more the `tombstone_warn_threshold` dead rows/tombstones
* a trace message, which includes stats of the amount of rows in the page, including the amount of live and dead rows as well as tombstones
This series extends this to also include information on cells, so we have visibility into the case where a read has to process an excessive amount of cell tombstones (mainly because of collections).
A log line is now also logged if the amount of dead cells/tombstones in the page exceeds `tombstone_warn_threshold`. The trace message is also extended to contain cell stats.
The `tombstone_warn_threshold` log lines now receive a 10s rate-limit to avoid excessive log spamming. The rate-limit is separate for the row and cell logs.
Example of the new log line (`tombstone_warn_threshold=10` ):
```
WARN 2024-05-30 07:56:44,979 [shard 0:stmt] querier - Read 98 live cells and 126 dead cells/tombstones for system_schema.scylla_tables <partition-range-scan> (-inf, +inf) (see tombstone_warn_threshold)
```
Example of the new tracing message:
```
Page stats: 1 partition(s), 0 static row(s) (0 live, 0 dead), 1 clustering row(s) (1 live, 0 dead), 0 range tombstone(s) and 13 cell(s) (1 live, 12 dead) [shard 0] | 2024-05-30 08:13:19.690803 | 127.0.0.1 | 6114 | 127.0.0.1
```
Fixes: https://github.com/scylladb/scylladb/issues/18996
Improvement, not a backport candidate.
Closesscylladb/scylladb#18997
* github.com:scylladb/scylladb:
test/boost: mutation_test: add test for cell compaction stats
mutation/compact_and_expire_result: drop operator bool()
querier: consume_page(): add rate-limiting to tombstone warnings
querier: consume_page(): add cell stats to page stats trace message
querier: consume_page(): add tombstone warning for cell tombstones
querier: consume_page(): extract code which logs tombstone warning
mutation/mutation_compactor: collect and aggregate cell compaction stats
mutation: row::compact_and_expire(): use compact_and_expire_result
collection_mutation: compact_and_expire(): use compact_and_expire_result
mutation: introduce compact_and_expire_result
This is a followup to #19937, for #19803. See in particular [this comment](https://github.com/scylladb/scylladb/issues/19803#issuecomment-2258371923).
The primary conversion target is coroutines. However, while coroutines are the most convenient style, they are only infrequently usable in this case, for the following reasons:
- Wherever we have a `future::finally()` that calls a cleanup function that returns a future (which must be awaited), we cannot use `co_await`. We can only use `seastar::async()` with `deferred_close` or `defer()`.
- The code passes lots of lambdas, and `co_await` cannot be used in lambdas. First, I tried, and the compiler rejects it; second, a capturing lambda that is a coroutine is a trap [[1]](https://devblogs.microsoft.com/oldnewthing/20211103-00/?p=105870) [[2]](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rcoro-capture).
In most cases, I didn't have to use naked `seastar::async()`; there were specialized wrappers in place already. Thus, most of the changes target `seastar::thread` context under existent `seastar::async()` wrappers, and only a few functions end up as coroutines.
The last patch in the series (`test/sstable: remove useless variable from promoted_index_read()`) is an independent micro-cleanup, the opportunity for which I thought to have noticed while reading the code.
The tail of `test/boost/sstable_test.cc` (the stuff following `promoted_index_read()`) is already written as `seastar::thread`. That's already better (for readability) than future chaining; but could have I perhaps further converted those functions to coroutines? My answer was "no":
- Some of the candidate functions relied on deferred cleanups that might need to yield (all three variants of `count_rows()`).
- Some had been implemented by passing lambdas to wrappers of `seastar::async()` (`sub_partition_read()`, `sub_partitions_read()`).
- The test case `test_skipping_in_compressed_stream()` initially looked promising for co-routinization (from its starting point `seastar::async()`), because it seemed to employ no deferred cleanup (that might need to yield). However, the function uses three lambdas that must be able to yield internally, and one of those (`make_is()`) is even capturing.
- The rest (`test_empty_key_view_comparison()`, `test_parse_path_good()`, `test_parse_path_bad()`) was synchronous code to begin with.
```
test/boost/sstable_test.cc | 188 +++++++++-----------
1 file changed, 83 insertions(+), 105 deletions(-)
```
Refactoring; no backport needed.
Closesscylladb/scylladb#20011
* github.com:scylladb/scylladb:
test/sstable: remove useless variable from promoted_index_read()
test/sstable: rewrite promoted_index_read() with async()
test/sstable: unfuturize lambda invocation in test_using_reusable_sst*()
test/sstable: rewrite wrong_range() with async()
test/sstable: simplify not_find_key_composite_bucket0() under test_using_reusable_sst()
test/sstable: rewrite full_index_search() with async()
test/sstable: simplify find_key*(), all_in_place() under test_using_reusable_sst()
test/sstable: rewrite (un)compressed_random_access_read() with async()
test/sstable: simplify write_and_validate_sst()
test/sstable: simplify check_toc_func() under async()
test/sstable: simplify check_statistics_func() under async()
test/sstable: simplify check_summary_func() under async()
test/sstable: coroutinize check_component_integrity()
test/sstable: rewrite write_sst_info() with async()
test/sstable: simplify missing_summary_first_last_sane()
test/sstable: coroutinize summary_query_fail()
test/sstable: rewrite summary_query() with async()
test/sstable: coroutinize (simple/composite)_index_read()
test/sstable: rewrite index_read() with async()
test/sstable: rewrite test_using_reusable_sst() with async()
test/sstable: rewrite test_using_working_sst() with async()
Test `test_auth_v2_migration` creates auth data where role `users`
has assigned service level `sl:fefe` but the service level isn't
actually created.
In following patches, we are going to introduce effective service levels
cache which depends on auth and is refreshed when mutations are applied
to v2 auth tables.
Without this changes, this test will fail because the service level
doesn't exist.
Also the name `sl:fefe` is change to `sl1`.
If tablet-based table is created concurrently with node being
decommissioned after tablets are already drained, the new table may be
permanently left with replicas on the node which is no longer in the
topology. That creates an immidiate availability risk because we are
running with one replica down.
This also violates invariants about replica placement and this state
cannot be fixed by topology operations.
One effect is that this will lead to load balancer failure which will
inhibit progress of any topology operations:
load_balancer - Replica 154b0380-1dd2-11b2-9fdd-7156aa720e1a:0 of tablet 7e03dd40-537b-11ef-9fdd-7156aa720e1a:1 not found in topology, at: ...
Fixes#20032Closesscylladb/scylladb#20053
Sync points are created, via POST HTTP requests, for a subset of nodes
in the cluster. Those nodes are specified in a request's parameter
`target_hosts`. When the parameter is empty, Scylla should assume
the user wants to create a sync point for ALL nodes.
Before these changes, sync points were created only for LIVE nodes.
If a node was dead but still part of the cluster and the user
requested creating a sync point leaving the parameter `target_hosts`
empty, the dead node was skipped during the creation of the sync point.
That was inconsistent with the guarantees the sync point API provides.
In this commit, we fix that issue and add a test verifying that
the changes have made the implementation compliant with the design
of the sync point API -- the test only passes after this commit.
Fixesscylladb/scylladb#9413Closesscylladb/scylladb#19750
Having an operator bool() on this struct is counter-intuitive, so this
commit drops it and migrates any remaining users to bool is_live().
The purpose of this operator bool() was to help in incrementally replace
the previous bool return type with compact_and_expire_result in the
compact_and_expire() call stack. Now that this is done, it has served
its purpose.
Add more logging for raft-based topology operations in INFO and DEBUG
levels.
Improve the existing logging, adding more details.
Fix a FIXME in test_coordinator_queue_management (by readding a log
message that was removed in the past -- probably by accident -- and
properly awaiting for it to appear in test).
Enable group0_state_machine logging at TRACE level in tests. These logs
are relatively rare (group 0 commands are used for metadata operations)
and relatively small, mostly consist of printing `system.group0_history`
mutation in the applied command, for example:
```
TRACE 2024-08-02 18:47:12,238 [shard 0: gms] group0_raft_sm - apply() is called with 1 commands
TRACE 2024-08-02 18:47:12,238 [shard 0: gms] group0_raft_sm - cmd: prev_state_id: optional(dd9d47c6-50ee-11ef-d77f-500b8e1edde3), new_state_id: dd9ea5c6-50ee-11ef-ae64-dfbcd08d72c3, creator_addr: 127.219.233.1, creator_id: 02679305-b9d1-41ef-866d-d69be156c981
TRACE 2024-08-02 18:47:12,238 [shard 0: gms] group0_raft_sm - cmd.history_append: {canonical_mutation: table_id 027e42f5-683a-3ed7-b404-a0100762063c schema_version c9c345e1-428f-36e0-b7d5-9af5f985021e partition_key pk{0007686973746f7279} partition_tombstone {tombstone: none}, row tombstone {range_tombstone: start={position: clustered, ckp{0010b4ba65c64b6e11ef8080808080808080}, 1}, end={position: clustered, ckp{}, 1}, {tombstone: timestamp=1722617232237511, deletion_time=1722617232}}{row {position: clustered, ckp{0010dd9ea5c650ee11efae64dfbcd08d72c3}, 0} tombstone {row_tombstone: none} marker {row_marker: 1722617232237511 0 0}, column description atomic_cell{ create system_distributed keyspace; create system_distributed_everywhere keyspace; create and update system_distributed(_everywhere) tables,ts=1722617232237511,expiry=-1,ttl=0}}}
```
note that the mutation contains a human-readable description of the
command -- like "create system_distributed keyspace" above.
These logs might help debugging various issues (e.g. when `apply` hangs
waiting for read_apply mutex, or takes too long to apply a command).
Ref: scylladb/scylladb#19105
Ref: scylladb/scylladb#19945Closesscylladb/scylladb#19998
Currently, the resource utilization in CI is low. Increasing the number of clusters will increase how many tests are executed simultaneously. This will decrease the time it takes to execute and improve resource utilization.
Related: https://github.com/scylladb/qa-tasks/issues/1667Closesscylladb/scylladb#19832
The command has a singl check for the missing keyspace and/or table
parameters and if the check fails, there is a combined error message.
Apparently this is confusing, so split the check so that missing
keyspace and missing table args have its own check and error message.
Fixes: scylladb/scylladb#19984Closesscylladb/scylladb#20005
rewrite the function as coroutine to make it easier to read and maintain, following lifetime issues we had and fixed in this function.
The second commit adds a test that drops a table while there is a counter update operation ongoing in the table.
The test reproduces issue https://github.com/scylladb/scylla-enterprise/issues/4475 and verifies it is fixed.
Follow-up to https://github.com/scylladb/scylladb/pull/19948
Doesn't require backport because the fix to the issue was already done and backported. This is just cleanup and a test.
Closesscylladb/scylladb#19982
* github.com:scylladb/scylladb:
db: test counter update while table is dropped
db: coroutinize do_apply_counter_update
Recently, some users have seen "Key size too large" errors in various
places. Cassandra and Scylla impose a 64KB length limit on keys, and
we have known about bugs in this area for a long time - and even had
some translated Cassandra unit tests that cover some of them. But these
tests did not cover all the corner cases and left us with partial and
fragmented knowledge of this problem, spread over many test files and
many issues.
In this patch, we add a single test file, test/cql-pytest/test_key_length.py
which attempts to rigourously explore the various bugs we have with
CQL key length limits. These test aim to reproduce all known bugs in
this area:
* Refs #3017 - CQL layer accepts set values too large to be written to
an sstable
* Refs #10366 - Enforce Key-length limits during SELECT
* Refs #12247 - Better error reporting for oversized keys during INSERT
* Refs #16772 - Key length should be limited to exactly 65535, not less
The following less interesting bug is already covered by many tests so
I decided not to test it again:
* Refs #7745 - Length of map keys and set items are incorrectly limited
to 64K in unprepared CQL
There's also a situation in materialized views and secondary indexes,
where a column that was _not_ a key, now becomes a key, and a length
limit needs to be enforced on it. We already have good test coverage
for this (in test/cql-pytest/test_secondary_index.py and in
test/cql-pytest/test_materialized_view.py), and we have an issue:
* Refs #8627 - Cleanly reject updates with indexed values where value > 64k
All 16 tests added here pass on Cassandra 5 except one that fails on
https://issues.apache.org/jira/browse/CASSANDRA-19270, but 11 of the
tests currently fail on Scylla (6 on #12247, 2 on #10366, 3 on #16772).
It is possible that our decision in #16772 will not be to fix Scylla
to match Cassandra but rather to declare that strict compatibility isn't
needed in this case or even that Cassandra is wrong. But even then,
having these tests which demonstrate the behavior of both Cassandra
and Scylla will be important.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#16779
