Instead of performing a rolling restart by calling `restart` in a loop
over every node in the cluster, use the dedicated
`manager.rolling_restart` function. This method waits until all other
nodes see the currently processed node as up or down before proceeding
to the next step. Not doing so may lead to surprising behavior.
In particular, in scylladb/scylladb#18369, a test failed shortly after
restarting three nodes. Because nodes were restarted one after another
too fast, when the third node was restarted it didn't send a
notification to the second node because it still didn't know that the
second node was alive. This led the second node to notice that the third
node restarted by observing that it incremented its generation in gossip
(it restarted too fast to be marked as down by the failure detector). In
turn, this caused the second node to send "third node down" and "third
node up" notifications to the driver in a quick succession, causing it
to drop and reestablish all connections to that node. However, this
happened _after_ rolling upgrade finished and _after_ the test logic
confirmed that all nodes were alive. When the notifications were sent to
the driver, the test was executing some statements necessary for the
test to pass - as they broke, the test failed.
Fixes: scylladb/scylladb#18369
because of https://bugzilla.redhat.com/show_bug.cgi?id=2278689,
the rebuilt abseil package provided by fedora has different settings
than the ones if the tree is built with the sanitizer enabled. this
inconsistency leads to a crash.
to address this problem, we have to reinstate the abseil submodule, so
we can built it with the same compiler options with which we build the
tree.
in this change
* Revert "build: drop abseil submodule, replace with distribution abseil"
* update CMake building system with abseil header include settings
* bump up the abseil submodule to the latest LTS branch of abseil:
lts_2024_01_16
* update scylla-gdb.py to adapt to the new structure of
flat_hash_map
This reverts commit 8635d24424.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18511
More than three years ago, in issue #7949, we noticed that trying to
set a `map<ascii, int>` from JSON input (i.e., using INSERT JSON or the
fromJson() function) fails - the ascii key is incorrectly parsed.
We fixed that issue in commit 75109e9519
but unfortunately, did not do our due diligence: We did not write enough
tests inspired by this bug, and failed to discover that actually we have
the same bug for many other key types, not just for "ascii". Specifically,
the following key types have exactly the same bug:
* blob
* date
* inet
* time
* timestamp
* timeuuid
* uuid
Other types, like numbers or boolean worked "by accident" - instead of
parsing them as a normal string, we asked the JSON parser to parse them
again after removing the quotes, and because unquoted numbers and
unquoted true/false happwn to work in JSON, this didn't fail.
The fix here is very simple - for all *native* types (i.e., not
collections or tuples), the encoding of the key in JSON is simply a
quoted string - and removing the quotes is all we need to do and there's
no need to run the JSON parser a second time. Only for more elaborate
types - collections and tuples - we need to run the JSON parser a
second time on the key string to build the more elaborate object.
This patch also includes tests for fromJson() reading a map with all
native key types, confirming that all the aforementioned key types
were broken before this patch, and all key types (including the numbers
and booleans which worked even befoe this patch) work with this patch.
Fixes#18477.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#18482
`boost::range::random_shuffle()` uses the deprecated
`std::random_shuffle()` under the hood, so let's use
`std::ranges::shuffle()` which is available since C++20.
this change should address the warning like:
```
[312/753] CXX build/debug/test/boost/counter_test.o In file included from test/boost/counter_test.cc:17:
/usr/include/boost/range/algorithm/random_shuffle.hpp:106:13: warning: 'random_shuffle<__gnu_cxx::__normal_iterator<counter_shard *, std::vector<counter_shard>>>' is deprecated: use 'std::shuffle' instead [-Wdepr
ecated-declarations]
106 | detail::random_shuffle(boost::begin(rng), boost::end(rng));
| ^
test/boost/counter_test.cc:507:27: note: in instantiation of function template specialization 'boost::range::random_shuffle<std::vector<counter_shard>>' requested here
507 | boost::range::random_shuffle(shards);
| ^
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/bits/stl_algo.h:4489:5: note: 'random_shuffle<__gnu_cxx::__normal_iterator<counter_shard *, std::vector<counter_shard>>>' has been explicitly marked
deprecated here
4489 | _GLIBCXX14_DEPRECATED_SUGGEST("std::shuffle")
| ^
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/x86_64-redhat-linux/bits/c++config.h:1957:45: note: expanded from macro '_GLIBCXX14_DEPRECATED_SUGGEST'
1957 | # define _GLIBCXX14_DEPRECATED_SUGGEST(ALT) _GLIBCXX_DEPRECATED_SUGGEST(ALT)
| ^
/usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/x86_64-redhat-linux/bits/c++config.h:1941:19: note: expanded from macro '_GLIBCXX_DEPRECATED_SUGGEST'
1941 | __attribute__ ((__deprecated__ ("use '" ALT "' instead")))
| ^
```
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18517
It's completely unused, likely in favor of recently added formatter
for the type in question.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#18502
{fmt} v10.0.0 introduces formatter for `std::optional`, so there
is no need to test it. furthermore the behavior of this formatter
is different from our homebrew one. so let's skip this test if
{fmt} v10.0.0 or up is used.
Refs #18508
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18509
in {fmt} version 10.0.0, it has a regression, which dropped the
formatter for `char *`, even it does format `const char*`, as the
latter is convertible to
`fmt::stirng_view`.
and this issue was addressed in 10.1.0 using 616a4937, which adds
the formatter for `Char *` back, where `Char` is a template parameter.
but we do need to print `vector<char*>`, so, to address the build
failure with {fmt} version 10.0.0, which is shipped along with
fedora 39. let's backport this formatter.
Fixes#18503
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18505
This series adds facilities to gently convert canonical mutations back to mutations
and to gently make canonical mutations or freeze mutations in a seastar thread.
Those are used in storage_service::merge_topology_snapshot to prevent reactor stalls
due to large mutation, as seed in the test_add_many_nodes_under_load dtest.
Also, migration_manager migration_request was converted to use a seastar thread
to use the above facilities to prevent reactor stalls with large schema mutations,
e,g, with a large number of tables, and/or when reading tablets mutations with
a large number of tablets in a table.
perf-simple-query --write results:
Before:
```
median 79151.53 tps ( 59.3 allocs/op, 16.0 logallocs/op, 14.3 tasks/op, 53289 insns/op, 0 errors)
```
After:
```
median 79716.73 tps ( 59.3 allocs/op, 16.0 logallocs/op, 14.3 tasks/op, 53314 insns/op, 0 errors)
```
Closesscylladb/scylladb#18290
* github.com:scylladb/scylladb:
storage_proxy: add mutate_locally(vector<frozen_mutation_and_schema>) method
raft: group0_state_machine: write_mutations_to_database: freeze mutations gently
database: apply_in_memory: unfreeze_gently large mutations
storage_service: get_system_mutations: make_canonical_mutation_gently
tablets: read_tablet_mutations: make_canonical_mutation_gently
schema_tables: convert_schema_to_mutations: make_canonical_mutation_gently
schema_tables: redact_columns_for_missing_features: get input mutation using rvalue reference
storage_service: merge_topology_snapshot: freeze_gently
canonical_mutation: add make_canonical_mutation_gently
frozen_mutation: move unfreeze_gently to async_utils
mutation: add freeze_gently
idl-compiler: generate async serialization functions for stub members
raft: group0_state_machine: write_mutations_to_database: use to_mutation_gently
storage_service: merge_topology_snapshot: co_await to_mutation_gently
canonical_mutation: add to_mutation_gently
idl-compiler: emit include directive in generated impl header file
mutation_partition: add apply_gently
collection_mutation: improve collection_mutation_view formatting
mutation_partition: apply_monotonically: do not support schema upgrade
test/perf: report also log_allocations/op
When a table has secondary indexes on *multiple* columns, and several
such columns are used for filtering in a query, Scylla chooses one
of these indexes as the main driver of the query, and the second
column's restriction is implemented as filtering.
Before this patch, the index to use was chosen fairly randomly, based on
the order of the indexes in the schema. This order may be different in
different coordinators, and may even change across restarts on the same
coordinators. This is not only inconsistent, it can cause outright wrong
results when using *paging* and switching (or restarting) coordinates
in the middle of a paged scan... One coordinator saves one index's key
in the paging state, and then the other coordinator gets this paging
state and wrongly believes it is supposed to be a key of a *different*
index.
The fix in this patch is to pick the index suitable for the first
indexed column mentioned in the query. This has two benefits over
the situation before the patch:
1. The decision of which index to use no longer changes between
coordinators or across restarts - it just depends on the schema
and the specific query.
2. Different indexes can have different "specificity" so using one
or the other can change the query's performance. After this patch,
the user is in control over which index is used by changing the
order of terms in the query. A curious user can use tracing to
check which index was used to implement a particular query.
An xfailing test we had for this issue no longer fails, so the "xfail"
marker is removed.
Fixes#7969
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#14450
write_mutations_to_database might need to handle
large mutations from system tables, so to prevent
reactor stalls, freeze the mutations gently
and call proxy.mutate_locally in parallel on
the individual frozen mutations, rather than
calling the vector<mutation> based entry point
that eventually freezes each mutation synchronously.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
To prevent stalls due to large schema mutations.
While at it, reserve the result canonical_mutation vector.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
The function upgrades the input mutation
only in certain cases. Currently it accepts
the input mutation by value, which may cause
and extraneous copy if the caller doesn't move
the mutation, as done in
`adjust_schema_for_schema_features`.
Getting an rvalue reference instead makes the
interface clearer.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Make a canonical mutation gently using an
async serialization function.
Similar to freeze_gently, yielding is considered
only in-between range tombstones and rows.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Unfreeze_gently doesn't have to be a method of
frozen_mutation. It might as well be implemented as
a free function reading from a frozen_mutation
and preparing a mutation gently.
The logic will be used in a later patch
to make a canonical mutation directly from
a frozen_mutation instead of unfreezing it
and then converting it to a canonical_mutation.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Allow yielding in between serializing of
range tombstones and rows to prevent reactor
stalls due to large mutations with many
rows or range tombstones.
mutations that have many cells might still
stall but those are considered infrequent enough
to ignore for now.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
The generated implementation header file depends
on the generated header file for the types it uses.
Generate a respective #include directive to make it self-sufficient.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Currently, if the input mutation_partition requires
schema upgrade, apply_monotonically always silently reverts to
being non-preemptible, even if the caller passed is_preemptible::yes.
To prevent that from happening, put the burden of upgrading
the mutation_partition schem on the caller, which is
today the apply() methods, which are synchronous anyhow.
With that, we reduce the proliferation of the
`apply_monotonically` overloads and keep only the
low level one (which could potentially be private as well,
as it's called only from within the mutation/ source files
and from tests)
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Different sstable storage backends use slightly different notion of what sstable location is. Filesystem storage knows it's `/var/lib/data/ks/cf-uuid/state` path, while s3 storage keeps only this path's part without state (and even that's not very accurate, because bucket prefix is missing as well as "/var/lib/data" prefix is not needed and eventually should be omitted). Nonetheless, the sstable_directory still keeps the filsystem-like path, while it's really only needed by the filesystem lister. This PR removes it.
Closesscylladb/scylladb#18496
* github.com:scylladb/scylladb:
sstable_directory: Remove _sstable_dir member
sstable_directory: Create sstable path with make_path() when logging
sstable_directory: Use make_path to construct filesystem lister
sstable_directory: Move some logging around
Following
b8634fb244
machine image started to fail with the following error:
```
10:44:59 ␛[0;32m googlecompute.gce: scylla-jmx package is not installed.␛[0m
10:44:59 ␛[1;31m==> googlecompute.gce: Traceback (most recent call last):␛[0m
10:44:59 ␛[1;31m==> googlecompute.gce: File "/home/ubuntu/scylla_install_image", line 135, in <module>␛[0m
10:44:59 ␛[1;31m==> googlecompute.gce: run('/opt/scylladb/scripts/scylla_setup --no-coredump-setup --no-sysconfig-setup --no-raid-setup --no-io-setup --no-ec2-check --no-swap-setup --no-cpuscaling-setup --no-ntp-setup', shell=True, check=True)␛[0m
10:44:59 ␛[1;31m==> googlecompute.gce: File "/usr/lib/python3.10/subprocess.py", line 526, in run␛[0m
10:44:59 ␛[1;31m==> googlecompute.gce: raise CalledProcessError(retcode, process.args,␛[0m
10:44:59 ␛[1;31m==> googlecompute.gce: subprocess.CalledProcessError: Command '/opt/scylladb/scripts/scylla_setup --no-coredump-setup --no-sysconfig-setup --no-raid-setup --no-io-setup --no-ec2-check --no-swap-setup --no-cpuscaling-setup --no-ntp-setup' returned non-zero exit status 1.␛[0m
```
It seems we no longer need to verify that jmx and tools-java packages are installed.
Closesscylladb/scylladb#18494
The sstable_directory::sstable_filename() should generate a name of an
sstable for log messages. It's not accurate, because it silently assumes
that the filename is on local storage, which might not be the case.
Fixing it is large chage, so for now replace _sstable_dir with explicit
call to make_path(). The change is idempotent, as _sstable_dir is
initialized with the result of make_path() call in constructor.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
At the beginning of .process() method there's a log message which path
and which storage is being processed. That's not really nice, because,
e.g. filesystem lister may skip processing quarantine directory. Also,
the registry lister doesn't list entries by their _sstable_dir, but
rather by its _location (spoiler: dir = location / state).
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
* seastar 2b43417d...b73e5e7d (11):
> treewide: inherit from formatter<string_view> not formatter<std::string_view>
> CMakeLists.txt: Apply CXX deprecated flags conditionally
> tls: add assignment operator for gnutls_datum
> tls: s/get0()/get()/
> io_queue: do not reference moved variable
> TLS: use helper function in get_distinguished_name & get_alt_name_information
> TLS: Add support for TLS1.3 session tickets
> iotune: ignore shards with id above max_iodepth
> core/future: remove a template parameter from set_callback()
> util: with_file_input_stream: always close file
> core/sleep: Use more raii-sh aproach to maintain sleeper
Fixes#5181Closesscylladb/scylladb#18491
Since we added native nodetool, we no longer need to install scylla-tools
and scylla-jmx, drop them from scylla metapackage and make it optional
package.
Closes#18472Closesscylladb/scylladb#18487
when compiling the tree with clang-18 and ragel 6.10, the compiler
warns like:
```
/usr/local/bin/cmake -E __run_co_compile --tidy="clang-tidy-18;--checks=-*,bugprone-use-after-move;--extra-arg-before=--driver-mode=g++" --source=/home/runner/work/scylladb/scylladb/redis/controller.cc -- /usr/bin/clang++-18 -DBOOST_NO_CXX98_FUNCTION_BASE -DSCYLLA_BUILD_MODE=release -DSEASTAR_API_LEVEL=7 -DSEASTAR_LOGGER_COMPILE_TIME_FMT -DSEASTAR_LOGGER_TYPE_STDOUT -DSEASTAR_SCHEDULING_GROUPS_COUNT=16 -DSEASTAR_SSTRING -DXXH_PRIVATE_API -I/home/runner/work/scylladb/scylladb -I/home/runner/work/scylladb/scylladb/build/gen -I/home/runner/work/scylladb/scylladb/seastar/include -I/home/runner/work/scylladb/scylladb/build/seastar/gen/include -I/home/runner/work/scylladb/scylladb/build/seastar/gen/src -isystem /home/runner/work/scylladb/scylladb/cooking/include -ffunction-sections -fdata-sections -O3 -g -gz -std=gnu++20 -fvisibility=hidden -Wall -Werror -Wextra -Wno-error=deprecated-declarations -Wimplicit-fallthrough -Wno-c++11-narrowing -Wno-deprecated-copy -Wno-mismatched-tags -Wno-missing-field-initializers -Wno-overloaded-virtual -Wno-unsupported-friend -Wno-enum-constexpr-conversion -Wno-unused-parameter -ffile-prefix-map=/home/runner/work/scylladb/scylladb=. -march=westmere -mllvm -inline-threshold=2500 -fno-slp-vectorize -U_FORTIFY_SOURCE -Werror=unused-result -MD -MT redis/CMakeFiles/redis.dir/controller.cc.o -MF redis/CMakeFiles/redis.dir/controller.cc.o.d -o redis/CMakeFiles/redis.dir/controller.cc.o -c /home/runner/work/scylladb/scylladb/redis/controller.cc
error: too many errors emitted, stopping now [clang-diagnostic-error]
Error: /home/runner/work/scylladb/scylladb/build/gen/redis/protocol_parser.hh:110:1: error: unannotated fall-through between switch labels [clang-diagnostic-implicit-fallthrough]
110 | case 1:
| ^
/home/runner/work/scylladb/scylladb/build/gen/redis/protocol_parser.hh:110:1: note: insert 'FMT_FALLTHROUGH;' to silence this warning
110 | case 1:
| ^
| FMT_FALLTHROUGH;
```
since we have `-Werror`, the warnings like this are considered as error,
hence the build fails. in order to address this failure, let's silence
this warning when including this generated header file.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18447
In our CDC implementation, the CDC log table for table "xyz" is always
called "xyz_scylla_cdc_log". If this table name is taken, and the user
tries to create a table "xyz" with CDC enabled - or enable CDC on the
table "xyz", the creation/enabling should fail gracefully, with a clear
error message. This test verifies this.
The new test passes - the code is already correct. I just wanted to
verify that it is (and to prevent future regressions).
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#18485
There are two places that workaround db.column_family_exists() call with some fancy exceptions-catching lambda.
This PR makes things simpler.
Closesscylladb/scylladb#18441
* github.com:scylladb/scylladb:
view: Open-code one line lambda checking if table exists
view: Use non-throwoing check if a table exists
because tracing/trace_keyspace_helper.cc references symbols
defined by table_helper, which is in turn provided by scylla-main,
we should link tracing_tracing against scylla-main.
otherwise we could have following link failure:
```
./build/./tracing/trace_keyspace_helper.cc:214: error: undefined reference to 'table_helper::setup_keyspace(cql3::query_processor&, service::migration_manager&, std::basic_string_view<char, std::char_traits<char> >, seastar::basic_sstring<char, unsigned int, 15u, true>, service::query_state&, std::vector<table_helper*, std::allocator<table_helper*> >)'
./build/./tracing/trace_keyspace_helper.cc:396: error: undefined reference to 'table_helper::cache_table_info(cql3::query_processor&, service::migration_manager&, service::query_state&)'
./table_helper.hh:92: error: undefined reference to 'table_helper::insert(cql3::query_processor&, service::migration_manager&, service::query_state&, seastar::noncopyable_function<cql3::query_options ()>)'
./table_helper.hh:92: error: undefined reference to 'table_helper::insert(cql3::query_processor&, service::migration_manager&, service::query_state&, seastar::noncopyable_function<cql3::query_options ()>)'
./table_helper.hh:92: error: undefined reference to 'table_helper::insert(cql3::query_processor&, service::migration_manager&, service::query_state&, seastar::noncopyable_function<cql3::query_options ()>)'
./table_helper.hh:92: error: undefined reference to 'table_helper::insert(cql3::query_processor&, service::migration_manager&, service::query_state&, seastar::noncopyable_function<cql3::query_options ()>)'
clang++-18: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
```
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18455
when building with CMake, there is a use case where the $BUILDIR
is not created yet, when `reloc/build_rpm.sh` is launched. in order
to enable us to run this script without creating $BUILDIR first, let's
create this directory first.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18464
clang-tidy warns like:
```
[628/713] Building CXX object service/CMakeFiles/service.dir/raft/raft_group_registry.cc.o
Warning: /home/runner/work/scylladb/scylladb/service/raft/raft_group_registry.cc:543:66: warning: 'id' used after it was moved [bugprone-use-after-move]
543 | auto& rate_limit = _rate_limits.try_get_recent_entry(id, std::chrono::minutes(5));
| ^
/home/runner/work/scylladb/scylladb/service/raft/raft_group_registry.cc:539:19: note: move occurred here
539 | auto dst_id = raft::server_id{std::move(id)};
| ^
```
this is a false alarm. as the type of `id` is actually `utils::UUID`
which is a struct enclosing two `int64_t` variables. and we don't
define a move constructor for `utils::UUID`. so the value of of `id`
is intact after being moved away. but it is still confusing at
the first glance, as we are indeed referencing a moved-away variable.
so in order to reduce the confusion and to silence the warning, let's
just do not `std::move(id)`.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18449
Doesn't test only coordinator ability to retry on failure, but also
that replica will be able to properly continue cleanup of a storage
group from where it left off (when failure happened), not leave any
sstables behind.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Closesscylladb/scylladb#18426
