Set the tombstone GC time for group0-managed tables to the minimal state
id of the group0 nodes.
The check is being done based on a timer, iterating through each node
(according to the group0 topology configuration) and taking the minimum
across all nodes.
This miminum timestamp is then be used to set the tombstone GC time
for the tombstone GC of all the group0-managed tables.
Fixes: scylladb/scylla#15607
Move the repair_map definition to the tombstone_gc file where it is
mostly being used.
Refactor and add the accessors and setters for the group0 tombstone GC
time.
Implemented the group0 state_id handler (based on the gossip) that will
broadcast the group0 state id of each node.
This will be used to set the tombstone GC time for the group0 tables.
Scylla doesn't allow for the types of arguments or the return type of a UDF
to be frozen. As a result, before these changes, create statements
produced to restore UDFs as part of `DESCRIBE` statements could not
be executed.
Fixesscylladb/scylladb#20256
Backport: necessary as the restore process may not work correctly without these changes. The affected versions span from 5.2 to the current master, but we only want to apply the fix to the live versions, so 6.0, 6.1, and 6.2.
Closesscylladb/scylladb#20816
* github.com:scylladb/scylladb:
cql3/functions/user_function: Print arguments and return type without frozen
cql3/functions/user_function: Use fmt to format create statement
This patch series fixes a couple of bugs around validating if RF is not changed by too much when performing ALTER tablets KS.
RF cannot change by more than 1 in total, because tablets load balancer cannot handle more work at once.
Fixes: #20039
Should be backported to 6.0 & 6.1 (wherever tablets feature is present), as this bug may break the cluster.
Closesscylladb/scylladb#20208
* github.com:scylladb/scylladb:
cql: sum of abs RFs diffs cannot exceed 1 in ALTER tablets KS
cql: join new and old KS options in ALTER tablets KS
cql: fix validation of ALTERing RFs in tablets KS
cql: harden `alter_keyspace_statement.cc::validate_rf_difference`
cql: validate RF change for new DCs in ALTER tablets KS
cql: extend test_alter_tablet_keyspace_rf
cql: refactor test_tablets::test_alter_tablet_keyspace
cql: remove unused helper function from test_tablets
Group0 server is often used in asynchronous context, but we do not wait
for them to complete before destroying the server. We already have
shutdown gate for it, so lets use it in those asynch functions.
Also make sure to signal group0 abort source if initialization fails.
Fixesscylladb/scylladb#20701
Backport to 6.2 since it contains af83c5e53e and it made the race easier to hit, so tests became flaky.
Closesscylladb/scylladb#20891
* github.com:scylladb/scylladb:
group: hold group0 shutdown gate during async operations
group0: Stop group0 if node initialization fails
Currently, `estimated_pending_compactions` uses a precalculated value calculated by `update_estimated_compaction_by_tasks`, which, in turn, is called by `get_compaction_candidates`. That means that, if `estimated_pending_compactions` is called, e.g. right after major compaction, it will return an outdated value that was calculated prior to major compaction, and so, it is no longer relevant.
Instead, just recalculate the value in `estimated_pending_compactions` and drop `update_estimated_compaction_by_tasks`.
* Enhancement, no backport required
Closesscylladb/scylladb#20892
* github.com:scylladb/scylladb:
test: cql-pytest: test_compaction: add test_compactionstats_after_major_compaction
test/cql-pytest: rename test_compaction{_tombstone_gc,}
time_window_compaction_strategy: estimated_pending_compactions: reestimate compactions rather than using cached value
storage_proxy::cancellable_write_handlers_list::update_live_iterators
assumes that iterators in _live_iterators can be dereferenced, but
the code does not make any attempt to make sure this is the case. The
iterator can be the end iterator which cannot be dereferenced.
The patch makes sure that there is no end iterator in _live_iterators.
Fixesscylladb/scylladb#20874Closesscylladb/scylladb#20977
Commit efd65aebb2 ("build: cmake: add check-header target", 2023-11-13)
introduced three typos:
- In "cmake/check_headers.cmake", it checked whether the
"parsed_args_GLOB_RECURSE" argument was defined, but then it referenced
the same under the wrong name "parsed_args_RECURSIVE".
- The above error masked two further typos; namely the duplicate use of
"api" and "streaming" each, as targets. With "parsed_args_GLOB_RECURSE"
above fixed, CMake now reports these conflicting arguments (target
names). They should have been "node_ops" and "sstables", respectively.
Correct the typos.
Signed-off-by: Laszlo Ersek <laszlo.ersek@scylladb.com>
Closesscylladb/scylladb#20992
Scylla doesn't allow for the types of arguments or the return type
to be frozen. As a result, before these changes, create statements
produced to restore UDFs as part of `DESCRIBE` statements could not
be executed.
We fix that and add a reproducer test and another one to verify that
the implementation is correct.
Before this patch, the "/localnodes" HTTP request to the Alternator server
lists all the live nodes of the current DC. This patch adds two optional
parameters to this query:
dc: allows to list the live nodes of a specific named DC instead of the
current DC of the server.
rack: allows to restrict the results to just the nodes belonging to a
specific named rack.
For both options, if no live node exists in the given dc or rack (in
particular, if such a dc or rack doesn't even exist), an empty list is
returned - it's not an error.
The default, if dc or rack is not specified - remains exactly as it is
today - look at the current DC (the one of the node being request), and
do not restrict the list to any specific rack.
We expect the new options that we added here to be useful for two use cases:
1. A client that knows of *some* Scylla node (belonging to an unknown DC),
but wants to list the nodes in *its* DC, which it knows by name.
2. A client in a multi-rack DC (e.g., multi-AZ region in AWS) that wants
to send requests to nodes in its own rack (which it knows by name),
to avoid cross-rack networking costs.
Note that in both cases, this requires clients to know the names of DCs
and AZs via some out-of-band means. The client can also get a list of DCs
and racks using the system.local system table, as the tests included in
this patch demonstrate.
This patch includes two set of tests for these new options: One in the the
single-node test/alternator framework that has a single dc and rack but
can still check the case of an unknown dc or rack (in which case an empty
list is returned). The second test is in the topology framework, and runs
an 8-node cluster with two DCs, two racks, and two nodes in each, and
checks all the combinations of "/localnodes" requests with and without
dc and rack options. This test also resolves a longstanding TODO that
asked for such a multi-DC test for "/localnodes" to be written.
Fixes#12147
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#20915
The helper makes sstables from env directly. Callers may not create the
factor after that. Less code the better.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#20983
since we only need the full definition of boost::regex in the .cc
file, where we
- define the constructor and destructor
- and actually use the regex.
there is no need to include boost/regex.hpp in the header, in order
to keep the preprocessed header smaller. let's use a header only
contains forward declarations in header, and include the full
definition in the .cc file.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Tablets load balancer is unable to process more than a single pending
replica, thus ALTER tablets KS cannot accept an ALTER statement which
would result in creating 2+ pending replicas, hence it has to validate
if the sum of absoulte differences of RFs specified in the statement is
not greter than 1.
A bug has been discovered while trying to ALTER tablets KS and
specifying only 1 out of 2 DCs - the not specified DC's RF has been
zeroed. This is because ALTER tablets KS updated the KS only with the
RF-per-DC mapping specified in the ALTER tablets KS statement, so if a
DC was ommitted, it was assigned a value of RF=0.
This commit fixes that plus additionally passes all the KS options, not
only the replication options, to the topology coordinator, where the KS
update is performed.
`initial_tablets` is a special case, which requires a special handling
in the source code, as we cannot simply update old initial_tablet's
settings with the new ones, because if only ` and TABLETS = {'enabled':
true}` is specified in the ALTER tablets KS statement, we should not zero the `initial_tablets`, but
rather keep the old value - this is tested by the
`test_alter_preserves_tablets_if_initial_tablets_skipped` testcase.
Other than that, the above mentioned testcase started to fail with
these changes, and it appeared to be an issue with the test not waiting
until ALTER is completed, and thus reading the old value, hence the
test's body has been modified to wait for ALTER to complete before
performing validation.
The validation has been corrected with:
1. Checking if a DC specified in ALTER exists.
2. Removing `REPLICATION_STRATEGY_CLASS_KEY` key from a map of RFs that
needs their RFs to be validated.
This function assumed that strings passed as arguments will be of
integer types, but that wasn't the case, and we missed that because this
function didn't have any validation, so this change adds proper
validation and error logging.
Arguments passed to this function were forwarded from a call to
`ks_prop_defs::get_replication_options`, which, among rf-per-dc mapping, returns also
`class:replication_strategy` pair. Second pair's member has been casted
into an `int` type and somehow the code was still running fine, but only
extra testing added later discovered a bug in here.
ALTER tablets KS validated if RF is not changed by more than 1 for DCs
that already had replicas, but not for DCs that didn't have them yet, so
specifying an RF jump from 0 to 2 was possible when listing a new DC in
ALTER tablets KS statement, which violated internal invariants of
tablets load balancer.
This PR fixes that bug and adds a multi-dc testcases to check if adding
replicas to a new DC and removing replicas from a DC is honoring the RF
change constraints.
Refs: #20039
1. Renamed the testcase to emphasize that it only focuses on testing
changing RF - there are other tests that test ALTER tablets KS
in general.
2. Fixed whitespaces according to PEP8
There are two issues in it. First, listing the registry with a consumer callback passes wrong argument to the consumer. Second, the primary key of the registry is wrong. Both issues don't show up, because existing tests that use mock don't read from it, only write. Tests that read from registry are python tests that start scylla and thus use real registry.
Closesscylladb/scylladb#20946
* github.com:scylladb/scylladb:
test: Use corrcet key in sstables registry mock
test: Pass entry status to mock registry consumer
This patch adds reproducer tests (still failing) for issue #20755, which
is about missing validation of materialized view names:
1. Unlike table and keyspace names which are limited to 48 characters,
we forgot to limit view name length, and an excessively long name can
cause Scylla to shut down :-(
2. Unlike table and keyspace names which only allow alphanumeric
characters, view names are missing this check and can include any
characters.
3. Luckily, even though we are missing the alphanumeric check, we at
least don't allow "/" in view names (if we allowed them, it could
allow users to write in any directory in the filesystem!). But when
this happens, we get an internal error instead of the expected
errors.
The first test also fails on Cassandra (it doesn't crash it, but leaves
the table in a strange state), but the other two pass.
Refs #20755
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#20761
Some adjustments to the `.clang-format` options to better match the current code:
* don't sort the include headers: causes large diffs especially in files with a lot of includes, and the `#include` ordering is not prescribed by the Seastar coding style
* binpack the arguments in function declarations and calls: allow binpacking (as opposed to forcing each parameter on a separate line if they don't fit into the line length)
* indented parameter continuation (as opposed to aligning to the open parenthesis) - aligning to the open parenthesis causes alignment issues especially with lambdas
Fixes: scylladb/scylladb#20951
No backport: Not a product issue, just applies to master.
Closesscylladb/scylladb#20968
* github.com:scylladb/scylladb:
clang-format: argument and function packing
clang-format: don't sort the include headers
simpler this way. `sst` does not help with the readability or
performance, but let's drop it. simpler this way. also, remove the
unused parameter.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#20961
these unused includes are identified by clang-include-cleaner.
after auditing the source files, all of the reports have been
confirmed.
please note, since we have `using seastar::shared_ptr` in
`seastarx.h`, this renders `#include <seastar/core/shared_ptr.hh>`
unnecessary if we don't need the full definition of `seastar::shared_ptr`.
so, in this change, all the unused includes are removed.
---
it's a cleanup, hence no need to backport.
Closesscylladb/scylladb#20963
* github.com:scylladb/scylladb:
.github: add db to iwyu's CLEANER_DIR
db: remove unused includes
in 787ea4b1, we introduced `_prefix` and `_sstables` member variables
to `sstables_loader::download_task_impl`, replacing the functionality
of `_snapshot_name`. However, we overlooked removing the now-obsolete
`_snapshot_name` variable.
this commit removes the unused `_snapshot_name` member variable to
improve code cleanliness and prevent potential confusion.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#20969
Test that compactionstats are empty, i.e.
there are no required compactions following
major compaction.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Currently, `estimated_pending_compactions` uses a precalculated value
calculated by `update_estimated_compaction_by_tasks`, which, in turn,
is called by `get_compaction_candidates`. That means that, if
`estimated_pending_compactions` is called, e.g. right after
major compaction, it will return an outdated value that was
calculated prior to major compaction, and so, it is no longer
relevant.
Instead, just recalculate the value in `estimated_pending_compactions`
and drop `update_estimated_compaction_by_tasks`.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Commit af83c5e53e moved aborting of group0 into the storage service
drain function. But it is not called if node fails during initialization
(if it failed to join cluster for instance). So lets abort on both
paths (but only once).
when compiling with clang-19 and the standard library from GCC-14.2,
we have:
```
/usr/bin/cmake -E __run_co_compile --tidy="clang-tidy;--checks=-*,bugprone-use-after-move;--extra-arg-before=--driver-mode=g++" --source=/__w/scylladb/scylladb/utils/bloom_filter.cc -- /usr/bin/clang++ -DBOOST_REGEX_DYN_LINK -DBOOST_REGEX_NO_LIB -DFMT_SHARED -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/__w/scylladb/scylladb -I/__w/scylladb/scylladb/seastar/include -I/__w/scylladb/scylladb/build/seastar/gen/include -I/__w/scylladb/scylladb/build/seastar/gen/src -ffunction-sections -fdata-sections -O3 -g -gz -std=gnu++23 -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=/__w/scylladb/scylladb/build=. -march=wes
Error: /__w/scylladb/scylladb/utils/bloom_filter.cc:81:1: error: unknown type name 'filter_ptr' [clang-diagnostic-error]
81 | filter_ptr create_filter(int hash, large_bitset&& bitset, filter_format format) {
| ^
Error: /__w/scylladb/scylladb/utils/bloom_filter.cc:82:12: error: no viable conversion from returned value of type '__detail::__unique_ptr_t<murmur3_bloom_filter>' (aka 'unique_ptr<utils::filter::murmur3_bloom_filter>') to function return type 'int' [clang-diagnostic-error]
82 | return std::make_unique<murmur3_bloom_filter>(hash, std::move(bitset), format);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Error: /__w/scylladb/scylladb/utils/bloom_filter.cc:85:1: error: unknown type name 'filter_ptr' [clang-diagnostic-error]
85 | filter_ptr create_filter(int hash, int64_t num_elements, int buckets_per, filter_format format) {
| ^
Error: /__w/scylladb/scylladb/utils/bloom_filter.cc:86:12: error: no viable conversion from returned value of type '__detail::__unique_ptr_t<murmur3_bloom_filter>' (aka 'unique_ptr<utils::filter::murmur3_bloom_filter>') to function return type 'int' [clang-diagnostic-error]
86 | return std::make_unique<murmur3_bloom_filter>(hash, large_bitset(get_bitset_size(num_elements, buckets_per)), format);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Error: /__w/scylladb/scylladb/utils/bloom_filter.hh:93:1: error: unknown type name 'filter_ptr' [clang-diagnostic-error]
93 | filter_ptr create_filter(int hash, large_bitset&& bitset, filter_format format);
| ^
Error: /__w/scylladb/scylladb/utils/bloom_filter.hh:94:1: error: unknown type name 'filter_ptr' [clang-diagnostic-error]
94 | filter_ptr create_filter(int hash, int64_t num_elements, int buckets_per, filter_format format);
| ^
Error: /__w/scylladb/scylladb/utils/i_filter.hh:17:25: error: no template named 'unique_ptr' in namespace 'std' [clang-diagnostic-error]
17 | using filter_ptr = std::unique_ptr<i_filter>;
| ~~~~~^
Error: /__w/scylladb/scylladb/utils/i_filter.hh:54:12: error: unknown type name 'filter_ptr' [clang-diagnostic-error]
54 | static filter_ptr get_filter(int64_t num_elements, double max_false_pos_prob, filter_format format);
| ^
4 warnings and 8 errors generated.
```
apparently, the definition of `std::unique_ptr` is missing where it is
used. so let's include `<memory>`, so that `i_filter.hh` is more
self-contained.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#20971
Commit aa1270a00c changed most uses
of `assert` in the codebase to `SCYLLA_ASSERT`.
But the comment fixed in this patch is talking specifically about
`assert`, and shouldn't have been changed. It doesn't make sense
after the change.
Closesscylladb/scylladb#20967
Currently, `cached_file::stream` (currently used only by index_reader,
to read index pages), works as follows.
Assume that the caller requested a read of the range [pos, pos + size).
Then:
- If the first page of the requested range is uncached,
the entire [pos, pos + size) range is read from disk (even if some
later pieces of it are cached), the resulting pages are added to the cache,
and the read completes (most likely) from the cached pages.
- If the first page of the read is cached, then the rest of the read
is handled page-by-page, in a sequential loop, serving each page
either from cache (if present) or from disk.
For example, assume that pages 0, 1, 2, 3, 4 are requested.
If exactly pages 1, 2 are cached, then `stream` will read the entire [0, 4] range
from disk and insert the missing 0, 3, 4, and then it will continue serving the
read from cache.
If exactly pages 0 and 3 are cached, then it will serve 0 from cache,
then it will read 1 from disk and insert it into cache,
then it will read 2 from disk and insert it into cache,
then it will serve 3 from cache,
then it will read 4 from disk and insert it into cache.
If exactly the first page is cached, a 128 kiB read turns
into 31 I/O sequential read ops.
This is weird, and doesn't look intended. In one case, we are reading even pages
we already have, just to avoid fragmenting the read, and in the other case
we are reading pages one-by-one (sequentially!) even if they are neighbours.
I'm not sure if cached_file should minimize IOPS or byte throughput,
but the current state is surely suboptimal. Even if its read strategy
is somehow optimal, it should still at least coalesce contiguous reads
and perform the non-contiguous reads in parallel.
This patch leans into minimizing IOPS. After the patch, we serve
as many front pages from the cache as we can, but when we see
an uncached page, we read the entire remainder of the read from disk.
As if we trimmed the read request by the longest cached prefix,
and then performed the rest using the logic from before the patch.
For example, if exactly pages 0 and 3 are cached,
then we serve 0 from cache,
then we read [1, 4] from disk and insert everything into cache.
For partially-cached files, this will result in more bytes read
from disk, but less IOPS. This might be a bad thing. But if so,
then we should lean the other way in a more explicit and efficient
way than we currently do.
Closesscylladb/scylladb#20935
Changes to better match the Seastar code style and the current codebase.
Allow parameter binpacking and continuation indenting.
Refs: scylladb/scylladb#20951
Sorting the include headers causes reordering of all headers and thus
large diffs, especially in the files that include a lot of headers that
have not been sorted before. This makes it harder to review the changes
and to understand the history of the file.
The Seastar code style doesn't prescribe any include headers ordering.
Refs: scylladb/scylladb#20951
these unused includes are identified by clang-include-cleaner.
after auditing the source files, all of the reports have been
confirmed.
please note, since we have `using seastar::shared_ptr` in
`seastarx.h`, this renders `#include <seastar/core/shared_ptr.hh>`
unnecessary if we don't need the full definition of `seastar::shared_ptr`.
so, in this change, all the unused includes are removed. but there are
some headers which are actually used, while still being identified by
this tool. these includes are marked with "IWYU pragma: keep".
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
The restore-from-s3 task uses load-and-stream internally which, in turn, unlinks loaded sstables on success. That's not what user expects when it restores from backup, objects should remain in bucket afterwards.
Closesscylladb/scylladb#20947
* github.com:scylladb/scylladb:
test: Add check that restored-from objects are not removed
sstables_loader: Dont unlink sstables when restoring from S3
sstables_loader: Make primary_replica_only bool_class RAII field
before this change, we enumerate the sstables tracked by the
system.sstables table, and restore them when serving
requests to "storage_service/restore" API. this works fine with
"storage_service/backup" API. but this "restore" API cannot be
used as a drop-in replacement of the rclone based API currently
used by scylla-manager.
in order to fill the gap, in this change:
* add the "prefix" parameter for specifying the shared prefix of
sstables
* add the "sstables" parameter for specifying the list of TOC
components of sstables
* remove the "snapshot" parameter, as we don't encode the prefix
on scylla's end anymore.
* make the "table" parameter mandatory.
Fixes https://github.com/scylladb/scylladb/issues/20461
----
this change is a part of the efforts to bring the native backup/restore to scylla, no need to backprt.
Closesscylladb/scylladb#20685
* github.com:scylladb/scylladb:
treewide: accept list of sstables in "restore" API
sstable: pass get_storage_option to sstable_directory::load_sstable()
test/nodetool: add body parameter to `expected_request`
tools/scylla-nodetool: enable nodetool to write HTTP body
During split prepare phase, there will be more than 1 compaction group with
overlapping token range for a given replica.
Assume tablet 1 has sstable A containing deleted data, and sstable B containing
a tombstone that shadows data in A.
Then split starts:
1) sstable B is split first, and moved from main (unsplit) group to a
split-ready group
2) now compaction runs in split-ready group before sstable A is split
tombstone GC logic today only looks at underlying group, so compaction is step
2 will discard the deleted data in A, since it belongs to another group (the
unsplit one), and so the tombstone can be purged incorrectly.
To fix it, compaction will now work with all uncompacting sstables that belong
to the same replica, since tombstone GC requires all sstables that possibly
contain shadowed data to be available for correct decision to be made.
Fixes https://github.com/scylladb/scylladb/issues/20044.
Branches 6.0, 6.1 and 6.2 are vulnerable, so backport is needed.
Closesscylladb/scylladb#20939
* github.com:scylladb/scylladb:
replica: Fix tombstone GC during tablet split preparation
service: Improve error handling for split
