Commit Graph

45457 Commits

Author SHA1 Message Date
Botond Dénes
d94591c260 Merge 'treewide: replace boost::find_if with std::ranges::find_if' from Kefu Chai
now that we are allowed to use C++23. we now have the luxury of using `std::ranges::find_if`.

in this change, we:

- replace `boost::find_if` with `std::ranges::find_if`
- remove all `#include <boost/range/algorithm/find_if.hpp>`

to reduce the dependency to boost for better maintainability, and leverage standard library features for better long-term support.

this change is part of our ongoing effort to modernize our codebase and reduce external dependencies where possible.

---

it's a cleanup, hence no need to backport.

Closes scylladb/scylladb#21495

* github.com:scylladb/scylladb:
  treewide: replace boost::find_if with std::ranges::find_if
  counters: replace boost::find_if with std::ranges::find_if
  combine.hh: use std::iter_const_reference_t when appropriate
2024-11-20 09:58:13 +02:00
Botond Dénes
075ca6cc02 Merge 'cql3: respect PER PARTITION LIMIT for aggregate queries' from Paweł Zakrzewski
Currently, PER PARTITION LIMIT is not implemented for aggregates and queries can result in more rows than expected from the same partition.

Instrument the result_set_builder class so that it can enforce PER PARTITION LIMIT for aggregate queries, specifically:
- add per_partition_limit to the result_set_builder
- expose the number of input rows in the selector

result_set_builder gets two new functions handling partition start and end:
- accept_partition_end for notifying that a partition has been finished. This is also called when a page ends, so we cannot simply flush here, as a naive implementation could do.
- accept_new_partition, where we flush_selectors() if it's indeed a new partition (and not a continuation of the previous) and the query has a grouping: we don't want to flush on new partition in a query like SELECT COUNT(*) FROM foo;

Fixes #5363

Closes scylladb/scylladb#21125

* github.com:scylladb/scylladb:
  test: enable PER PARTIION LIMIT + GROUP BY tests
  cql3: respect PER PARTITION LIMIT for aggregates
  cql3: selection: count input rows in the selector
  cql3: selection: pass per partition limit to the result_set_builder
  cql3: show different messages for LIMIT and PER PARTITION LIMIT in get_limit
2024-11-20 09:54:28 +02:00
Botond Dénes
5ccbd500e0 Merge 'repair: fix task_manager_module::abort_all_repairs' from Aleksandra Martyniuk
Currently, task_manager_module::abort_all_repairs marks top-level repairs as aborted (but does not abort them) and aborts all existing shard tasks.

A running repair checks whether its id isn't contained in _aborted_pending_repairs and then proceeds to create shard tasks. If abort_all_repairs is executed after _aborted_pending_repairs is checked but before shard tasks are created, then those new tasks won't be aborted. The issue is the most severe for tablet_repair_task_impl that checks the _aborted_pending_repairs content from different shards, that do not see the top-level task. Hence the repair isn't stopped but it creates shard repair tasks on all shards but the one that initialized repair.

Abort top-level tasks in abort_all_repairs. Fix the shard on which the task abort is checked.

Fixes: #21612.

Needs backport to 6.1 and 6.2 as they contain the bug.

Closes scylladb/scylladb#21616

* github.com:scylladb/scylladb:
  test: add test to check if repair is properly aborted
  repair: add shard param to task_manager_module::is_aborted
  repair: use task abort source to abort repair
  repair: drop _aborted_pending_repairs and utilize tasks abort mechanism
  repair: fix task_manager_module::abort_all_repairs
2024-11-20 06:43:01 +02:00
Nadav Har'El
733a4f94c7 Merge 'test/boost/view_schema_test.cc: Wait for views to build in test_view_update_generating_writetime' from Dawid Mędrek
Before these changes, we didn't wait for the materialized views to
finish building before writing to the base table. That led to generating
an additional view update, which, in turn, led to test failures.

The scenario corresponding to the summary above looked like this:

1. The test creates an empty table and MVs on it.
2. The view builder starts, but it doesn't finish immediately.
3. The test performs mutations to the base table. Since the views
   already exist, view updates are generated.
4. Finally, the view builder finishes. It notices that the base
   table has a row, so it generates a view update for it because
   it doesn't notice that we already have data in the view.

We solve it by explicitly waiting for both views to finish building
and only then start writing to the base table.

Additionally, we also fix a lifetime issue of the row the test revolves
around, further stabilizing CI.

Fixes https://github.com/scylladb/scylladb/issues/20889

Backport: These changes have no semantic effect on the codebase,
but they stabilize CI, so we want to backport them to the maintained
versions of Scylla.

Closes scylladb/scylladb#21632

* github.com:scylladb/scylladb:
  test/boost/view_schema_test.cc: Increase TTL in test_view_update_generating_writetime
  test/boost/view_schema_test.cc: Wait for views to build in test_view_update_generating_writetime
2024-11-19 18:10:52 +02:00
Dawid Mędrek
af4afc84ec test/boost/view_schema_test.cc: Increase TTL in test_view_update_generating_writetime
The auxiliary function `eventually()` (defined in `test/lib/eventually.hh`)
tries to execute a passed function. If it throws, `eventually()` sleeps
for `2^#previous_attempts` milliseconds and tries to perform it again.
The default limit of attempts is 17.

In `test_view_update_generating_writetime`, right before the last test
case, we perform:

```cql
UPDATE t USING TTL 10 AND TIMESTAMP 8 SET g=40 WHERE k=1 AND c=1;
```

The test case itself executes:

```cql
SELECT WRITETIME(g) FROM t;
```

and asserts that the result of the query is equal to 8, i.e. it
corresponds to the timestamp of the last write to the table `t`.

However, if the test case keeps failing, then during its 14th attempt
(so affter sleeping for at least `2^14 - 1` milliseconds, which amounts
to about 16 seconds), we'll observe the following error:

```
[Exception] - std::runtime_error: Expected row not found: [0000000000000008] not in {result_message::rows {row:  null}}
```

The reason behind it is the specified TTL is too short. 10 seconds will
have already passed before the 14th attempt, so the value in the column
`g` will be `NULL` again. In particular, the `WRITETIME(g)` will no
longer be equal to `8`.

To solve that issue, we change the TTL in the CQL statement to 300.
The time spent on 17 loops of `eventually()` amounts to about
`2^18 - 1` milliseconds, which is about 263 seconds. That's why
setting the TTL to 300 seconds should be enough to prevent the error
from occurring.
2024-11-19 13:02:34 +01:00
Dawid Mędrek
5ca0cc4e85 test/boost/view_schema_test.cc: Wait for views to build in test_view_update_generating_writetime
Before these changes, we didn't wait for the materialized views to
finish building before writing to the base table. That led to generating
an additional view update, which, in turn, led to test failures.

The scenario corresponding to the summary above looked like this:

1. The test creates an empty table and MVs on it.
2. The view builder starts, but it doesn't finish immediately.
3. The test performs mutations to the base table. Since the views
   already exist, view updates are generated.
4. Finally, the view builder finishes. It notices that the base
   table has a row, so it generates a view update for it because
   it doesn't notice that we already have data in the view.

We solve it by explicitly waiting for both views to finish building
and only then start writing to the base table.

Fixes scylladb/scylladb#20889
2024-11-19 12:51:22 +01:00
Aleksandra Martyniuk
f5795e8aa4 test: add test to check if repair is properly aborted 2024-11-19 11:59:29 +01:00
Paweł Zakrzewski
b893e63b4a test: enable PER PARTIION LIMIT + GROUP BY tests 2024-11-19 09:28:01 +01:00
Nadav Har'El
7607f5e33e alternator: fix "/localnodes" to not return down nodes
Alternator's "/localnodes" HTTP requests is supposed to return the list
of nodes in the local DC to which the user can send requests.

Before commit bac7c33313 we used the
gossiper is_alive() method to determine if a node should be returned.
That commit changed the check to is_normal() - because a node can be
alive but in non-normal (e.g., joining) state and not ready for
requests.

However, it turns out that checking is_normal() is not enough, because
if node is stopped abruptly, other nodes will still consider it "normal",
but down (this is so-called "DN" state). So we need to check **both**
is_alive() and is_normal().

This patch also adds a test reproducing this case, where a node is
shut down abruptly. Before this patch, the test failed ("/localnodes"
continued to return the dead node), and after it it passes.

Fixes #21538

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes scylladb/scylladb#21540
2024-11-19 10:04:59 +02:00
Yaron Kaikov
980f6a48ab .github/scripts/auto-backport.py: validate backport candidate with Fixes prefix
Adding `Fixes` validation to a PR when backport labels were added. When the auto backport process triggers (after promotion), we will ensure each PR with backport/x.y label also has in the PR body a `Fixes` reference to an issue

Fixes: https://github.com/scylladb/scylladb/issues/20021

Closes scylladb/scylladb#21563
2024-11-19 09:48:34 +02:00
Benny Halevy
165902b951 conf/scylla.yaml: update documentation for enable_tablets
Change e3e8a94c9a changed
the semantics of the enable_tablets config option,
but updating that in the option documentation in scylla.yaml
was missed.

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>

Closes scylladb/scylladb#21614
2024-11-19 09:44:53 +02:00
Botond Dénes
36870feb29 Merge 'test: route S3 Proxy server messages through logger' from Kefu Chai
This change was created in the same spirit of f8221b960f.
The S3ProxyServer (introduced in 8919e0abab) currently prints its
status directly to stdout, which can be distracting when reviewing test
results. For example:

```console
$ ./test.py --mode release object_store/test_backup::test_simple_backup_and_restore
Found 1 tests.
Setting minio proxy random seed to 1731924995
Starting S3 proxy server on ('127.193.179.2', 9002)
================================================================================
[N/TOTAL]   SUITE    MODE   RESULT   TEST
------------------------------------------------------------------------------
[1/1]      object_store release [ PASS ] object_store.test_backup.1
Stopping S3 proxy server
------------------------------------------------------------------------------
CPU utilization: 3.1%
```

Move these messages to use proper logging to give developers more control
over their visibility:

- Make logger parameter mandatory in S3ProxyServer constructor
- Route "Stopping S3 proxy" message through the provided logger
- Add --log-level option to the standalone proxy server launcher

The message is now hidden:

```console
$ ./test.py --mode release object_store/test_backup::test_simple_backup_and_restore
Found 1 tests.
================================================================================
[N/TOTAL]   SUITE    MODE   RESULT   TEST
------------------------------------------------------------------------------
[1/1]      object_store release [ PASS ] object_store.test_backup.1
------------------------------------------------------------------------------
CPU utilization: 4.1%
```

---

this change improves the developer experience, hence no need to backport.

Closes scylladb/scylladb#21610

* github.com:scylladb/scylladb:
  test: route S3 Proxy server messages through logger
  test: s3_proxy: remove unused method
2024-11-19 06:42:28 +02:00
Kefu Chai
33a0e5b892 treewide: replace boost::find_if with std::ranges::find_if
now that we are allowed to use C++23. we now have the luxury of using
`std::ranges::find_if`.

in this change, we:

- replace `boost::find_if` with `std::ranges::find_if`
- remove all `#include <boost/range/algorithm/find_if.hpp>`

to reduce the dependency to boost for better maintainability, and
leverage standard library features for better long-term support.

this change is part of our ongoing effort to modernize our codebase
and reduce external dependencies where possible.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2024-11-19 10:50:01 +08:00
Kefu Chai
3e75fbd9d3 counters: replace boost::find_if with std::ranges::find_if
std::ranges allows us to create a range from a pair of iterators.
but the iterator has to fulfill the concept of `std::semiregular`.
in order to reduce the header dependency on boost, we need to
make `basic_counter_cell_view::shard_iterator` to support
`std::semiregular`.

in this change:

- define a default constructor for
  `basic_counter_cell_view::shard_iterator`, so that the iterator
  satisfies the constraints of `std::semiregular`, as required by
  C++20's forward_iterator concept. please note, despite that
  the standard requires the iterator to be `std::semiregular`, but
  the iterator created by default constructor is not evaluated in
  production. sometimes, the standard algorithms just need to
  store/create itermediate iterators or to represent a "singular"
  state for iterator. a use case is an empty container.
- change `basic_counter_cell_view::shard_iterator::reference` so
  its dereference returns a rvalue instead of a reference. because
  per C++20 standard, the dereference of a forward_iterator should
  be stable, but we were returning a reference / pointer referencing
  a member variable of the iterator. so once the iterator is destructed,
  the returned reference / pointer would be invalidated. so we have to
  return a value to fulfill the requiremend of forward_iterator. this
  change also fulfills the requirement of `same_as<iter_reference_t<It>,
  iter_reference_t<const It>>`, which a part of the
  `indirectly_readable` requirement.
- let `basic_counter_cell_view::shards()` return a subrange
- let `basic_counter_shard_view::swap_value_and_clock()` accepts
  a plain value instead of a reference. because the dereference of
  the iterator does not return a reference anymore. and the returned
  type is a lightweighted "view", so the performance penality is
  negligible.
- use ranges libraries when appropriate in this header.

this change is part of our ongoing effort to modernize our codebase
and reduce external dependencies where possible.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2024-11-19 10:50:01 +08:00
Kefu Chai
69939ee653 combine.hh: use std::iter_const_reference_t when appropriate
before this change, we assumed that the dereference types of
the given `InputIterator1` and `InputIterator2` are always
references. but this does not hold if the `operator*` returns
a rvalue, as in the C++20 standard, unlike the LegacyForwardIterator
requirement, `std::forward_iterator` does not requires
dereference to return a reference. so we should not assume this,
if we want to use `combine()` with iterators whose dereference
return a, for instance, rvalue.

in this change, we use `std::iter_const_reference_t` instead. this
type is deduced from the behavior of the iterator instead of hardwire
it to a reference type. this allows us to use a C++20 forward_iterator
with this generic function.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2024-11-19 10:50:01 +08:00
Avi Kivity
b14871ad3f Merge 'code cleanup: remove "sstring_view" and replace its usages by std::string_view' from Nadav Har'El
For historic reasons, we have (in bytes.hh) a type sstring_view which is an alias for std::string_view - since the same standard type can hold a pointer into both a seastar::sstring and std::string.

This alias in unnecessary and misleading to new developers, who might be misled to believe it is assume it is somehow different from std::string_view - when it isn't.

This series removes all uses of sstring_view (changing them to use std::string_view), and in the last patch removes the alias itself. A few functions whose name referred to "sstring" but take a std::string_view were renamed.

The patches are fairly mechanical and trivial, with no functional changes intended. To ease the review the series was split to a few smaller patches that modify specific areas of the code.

Fixes #4062.

Closes scylladb/scylladb#21617

* github.com:scylladb/scylladb:
  bytes: remove unused alias sstring_view
  change remaining sstring_view to std::string_view
  test: change sstring_view to std::string_view
  cql3: change sstring_view to std::string_view
  alternator: change sstring_view to std::string_view
  type: change from_sstring() to from_string_view()
  cross-tree: change to_sstring_view() to to_string_view()
2024-11-18 22:43:46 +02:00
Tomasz Grabiec
06d478793d Merge 'mutation: switch from boost ranges to std ranges' from Avi Kivity
Wean the mutation code (at least the headers) from boost ranges to std ranges,
in order to reduce the dependency load.

Cleanup, so no backport.

Closes scylladb/scylladb#21601

* github.com:scylladb/scylladb:
  partition_snapshot_row_cursor.hh: switch from boost ranges to std ranges
  mutation: mutation_partition_v2.hh: switch from boost ranges to std ranges
  mutation: mutation_partition.hh: switch from boost ranges to std ranges
  partition_snapshot_reader.hh: drop unused include boost/range/algorithm/heap_algorithm.hpp
2024-11-18 21:23:29 +01:00
Luis Freitas
34d7a4401d ./github/workflows/conflict_reminder.yaml: fix assignee object
References the login property of object assignee

Closes scylladb/scylladb#21615
2024-11-18 19:42:58 +02:00
Paweł Zakrzewski
08eb853a96 cql3: respect PER PARTITION LIMIT for aggregates
This change adds support for PER PARTITION LIMIT for aggregate queries.
result_set_builder gets two new functions handling partition start and
end:
- accept_partition_end for notifying that a partition has been finished.
  This is also called when a page ends, so we cannot simply flush here,
  as a naive implementation could do.
- accept_new_partition, where we flush_selectors() if it's indeed a new
  partition (and not a continuation of the previous) and the query has a
  grouping: we don't want to flush on new partition in a query like
  SELECT COUNT(*) FROM foo;
2024-11-18 17:56:53 +01:00
Paweł Zakrzewski
8190d76dd6 cql3: selection: count input rows in the selector
This will allow result_set_builder::flush_selectors() to only flush when
there are input rows.
2024-11-18 17:56:53 +01:00
Paweł Zakrzewski
aea3c3851e cql3: selection: pass per partition limit to the result_set_builder
Aggregates require the limit to be applied from within the builder
class, so it needs to be passed to it.
2024-11-18 17:56:53 +01:00
Paweł Zakrzewski
cb1483037c cql3: show different messages for LIMIT and PER PARTITION LIMIT in get_limit
select_statement::get_limit is used to evaluate the LIMIT value for both
LIMIT and PER PARTITION LIMIT. This change fixes the error message for
incorrect values passed by the user.
2024-11-18 17:56:53 +01:00
Aleksandra Martyniuk
ca14167b20 repair: add shard param to task_manager_module::is_aborted
Currently, task_manager_module::is_aborted checks whether a task
with given id was aborted on this shard.

In tablet_repair_task_impl::run, is_aborted method is called on all
shards to check if the parent task was aborted. However, even
for aborted parent, is_aborted will return true only on owner shard
of the parent.

Pass shard param to task_manager_module::is_aborted that indicates
which shard to check.
2024-11-18 16:26:08 +01:00
Aleksandra Martyniuk
de8d59172a repair: use task abort source to abort repair
Aborting of a top-level repair does not need task_mananger_module
anymore. Use task's abort source wherever possible.
2024-11-18 16:26:08 +01:00
Aleksandra Martyniuk
a6d9931705 repair: drop _aborted_pending_repairs and utilize tasks abort mechanism 2024-11-18 16:26:08 +01:00
Aleksandra Martyniuk
db8308fe93 repair: fix task_manager_module::abort_all_repairs
Currently, task_manager_module::abort_all_repairs marks top-level
repairs as aborted (but does not abort them) and aborts all shard
tasks. If after that a top-level repair creates a shard task,
the new shard repair won't be aborted.

Abort top-level repair tasks in abort_all_repairs. They will abort
their children and newly created shard tasks will be immediately
aborted.
2024-11-18 16:25:57 +01:00
Nadav Har'El
5e20cb8c66 bytes: remove unused alias sstring_view
Our "sstring_view" was an historic alias for the standard std::string_view.
All its uses were removed in the previous patches, so we can now finally
remove this unused alias.

Refs #4062.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
2024-11-18 16:51:15 +02:00
Nadav Har'El
e639434a89 change remaining sstring_view to std::string_view
Our "sstring_view" is an historic alias for the standard std::string_view.
The patch changes the last remaining random uses of this old alias across
our source directory to the standard type name.

After this patch, there are no more uses of the "sstring_view" alias.
It will be removed in the following patch.

Refs #4062.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
2024-11-18 16:48:57 +02:00
Nadav Har'El
e72aabae7f test: change sstring_view to std::string_view
Our "sstring_view" is an historic alias for the standard std::string_view.
The test/ directory used this old alias in a few of random places, let's
change them to use the standard type name.

Refs #4062.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
2024-11-18 16:26:20 +02:00
Nadav Har'El
b778ce08a9 cql3: change sstring_view to std::string_view
Our "sstring_view" is an historic alias for the standard std::string_view.
The cql3/ directory used this old alias in a few of random places, let's
change them to use the standard type name.

Refs #4062.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
2024-11-18 15:57:20 +02:00
Nadav Har'El
f2b4a59ec7 alternator: change sstring_view to std::string_view
Our "sstring_view" is an historic alias for the standard std::string_view.
Alternator only used this alias in a couple of random names, let's change
them to the standard type name.

Refs #4062.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
2024-11-18 15:44:49 +02:00
Nadav Har'El
766ee56536 type: change from_sstring() to from_string_view()
All CQL type implementations have a from_sstring(sstring_view) method.
The "sstring_view" type is just an historic alias for std::string_view,
so this patch switches to use the standard type as suggested in #4062,
and also renames these functions from_string_view() to emphesize they can
take any string view, and not necessarily a "sstring" as their old name
suggested.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
2024-11-18 15:33:04 +02:00
Nadav Har'El
da99dc3a7f cross-tree: change to_sstring_view() to to_string_view()
For historic reasons, we have (in bytes.hh) a type sstring_view which
is an alias for std::string_view - since the same standard type can hold
a pointer into both a seastar::sstring and std::string.

This alias in unnecessary and misleading to new developers (who might
assume it is somehow different from std::string_view). This patch doesn't
yet remove all occurances of sstring_view (the request in #4062), but
begins to do it by renaming one commonly-used function, to_sstring_view(bytes)
to to_string_view() and of course changes all its uses to the new name.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
2024-11-18 14:57:49 +02:00
Kefu Chai
cb24022b54 test: route S3 Proxy server messages through logger
This change was created in the same spirit of f8221b960f.
The S3ProxyServer (introduced in 8919e0abab) currently prints its
status directly to stdout, which can be distracting when reviewing test
results. For example:
```console
$ ./test.py --mode release object_store/test_backup::test_simple_backup_and_restore
Found 1 tests.
Setting minio proxy random seed to 1731924995
Starting S3 proxy server on ('127.193.179.2', 9002)
================================================================================
[N/TOTAL]   SUITE    MODE   RESULT   TEST
------------------------------------------------------------------------------
[1/1]      object_store release [ PASS ] object_store.test_backup.1
Stopping S3 proxy server
------------------------------------------------------------------------------
CPU utilization: 3.1%
```

Move these messages to use proper logging to give developers more control
over their visibility:

- Make logger parameter mandatory in S3ProxyServer constructor
- Route "Stopping S3 proxy" message through the provided logger
- Add --log-level option to the standalone proxy server launcher

The message is now hidden:

```console
$ ./test.py --mode release object_store/test_backup::test_simple_backup_and_restore
Found 1 tests.
================================================================================
[N/TOTAL]   SUITE    MODE   RESULT   TEST
------------------------------------------------------------------------------
[1/1]      object_store release [ PASS ] object_store.test_backup.1
------------------------------------------------------------------------------
CPU utilization: 4.1%
```

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2024-11-18 18:41:17 +08:00
Kefu Chai
0dff187b7a test: s3_proxy: remove unused method
neither `InjectingHandler.log_error`, nor `InjectingHandler.log_message`
is used. so let's drop them.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2024-11-18 18:39:15 +08:00
Aleksandra Martyniuk
572b005774 repair: implement tablet_repair_task_impl::release_resources
tablet_repair_task_impl keeps a vector of tablet_repair_task_meta,
each of which keeps an effective_replication_map_ptr. So, after
the task completes, the token metadata version will not change for
task_ttl seconds.

Implement tablet_repair_task_impl::release_resources method that clears
tablet_repair_task_meta vector when the task finishes.

Set task_ttl to 1h in test_tablet_repair to check whether the test
won't time out.

Fixes: #21503.

Closes scylladb/scylladb#21504
2024-11-18 12:29:58 +02:00
Avi Kivity
bef015da0d Revert ".github/scripts/auto-backport.py: validate backport candidate with Fixes prefix"
This reverts commit 8414cd743e. It prevents
pulling pull requests that do have "Fixes" references.
2024-11-18 12:29:22 +02:00
Avi Kivity
3a6c0a9b36 Merge 'compaction: Perform integrity checks on compacting SSTables' from Nikos Dragazis
This PR enables compaction tasks to verify the integrity of the input data through checksum and digest checks. The mechanism for integrity checking was introduced in previous PRs (#20207, #20720) as a built-in functionality of the input streams. This PR integrates this mechanism with compaction. The change applies to all compaction types and covers both compressed and uncompressed SSTables adhering to the 3.x format. If a compaction task reads only part of an SSTable, then only the per-chunk checksums are verified, not the digest.

The PR consists of:
* Changes to mx readers to support integrity checking. The kl readers, considered as compatibility-only, were left unchanged. Also, integrity checking on single-partition reversed reads (`data_consume_reversed_partition()`) remains unsupported by mx readers as this is not used in compaction.
* Changes to `sstable` and `sstable_set` APIs to allow toggling integrity checks for mx readers.
* Activation of integrity checking for all compaction types.
* Tests for all compaction types with corrupted SSTables.

Integrity checks come at a cost. For uncompressed SSTables, the cost is the loading of the CRC and Digest components from disk, and the calculation of checksums and digest from the actual data. For compressed SSTables, checksums are stored in-place and they are being checked already on all reads, so the only extra cost is the loading and calculation of the digest. The measurements show a ~5% regression in compaction performance for uncompressed SSTables, and a negligible regression for compressed SSTables.

Command: `perf-sstable --smp=1 --cpuset=1 --poll-mode --mode=compaction --iterations=1000 --partitions 10000 --sstables=1 --key_size=4096 --num_columns=15 --column_size={32, 1024, 3500, 7000, 14500}`

Uncompressed SSTables:
```
+--------------+-----------------------+----------------------+------------+
| SSTable Size | No Integrity (p/sec)  | Integrity (p/sec)    | Regression |
+--------------+-----------------------+----------------------+------------+
| 50  MiB      | 65175.59 +- 80.82     | 61814.63 +- 72.88    | 5.16%      |
| 200 MiB      | 41795.10 +- 60.39     | 39686.28 +- 45.05    | 5.05%      |
| 500 MiB      | 21087.41 +- 30.72     | 20092.93 +- 25.05    | 4.72%      |
| 1   GiB      | 12781.64 +- 21.77     | 12233.94 +- 21.71    | 4.29%      |
| 2   GiB      |  6629.99 +-  9.40     |  6377.13 +-  8.28    | 3.81%      |
+--------------+-----------------------+----------------------+------------+
```
Compressed SSTables:
```
+--------------+-----------------------+----------------------+------------+
| SSTable Size | No Integrity (p/sec)  | Integrity (p/sec)    | Regression |
+--------------+-----------------------+----------------------+------------+
| 50  MiB      | 53975.05 +- 63.18     | 53825.93 +- 62.28    |  0.28%     |
| 200 MiB      | 28687.94 +- 26.58     | 28689.41 +- 26.91    |  0%        |
| 500 MiB      | 13865.35 +- 15.50     | 13790.41 +- 14.88    |  0.54%     |
| 1   GiB      |  7858.10 +-  7.71     |  7829.75 +-  9.66    |  0.36%     |
| 2   GiB      |  4023.11 +-  2.43     |  4010.54 +-  2.55    |  0.31%     |
+--------------+-----------------------+----------------------+------------+
(p/sec = partitions/sec)
```

Refs #19071.

New feature, no backport is needed.

Closes scylladb/scylladb#21153

* github.com:scylladb/scylladb:
  test: Add test for compaction with corrupted SSTables
  compaction: Enable integrity checks for all compaction types
  sstables: Add integrity option to factories for sstable_set readers
  sstables: Add integrity option to sstable::make_reader()
  sstables: Add integrity option to mx::make_reader()
  sstables: Load checksums and digests in mx full-scan reader
  sstables: Add integrity option to data_consume_single_partition()
  sstables: Disengage integrity_check from sstable class
  sstables: Allow data sources to disable digest check
2024-11-17 20:59:31 +02:00
Nadav Har'El
f23800181a Merge 'Align Metric Family Descriptions' from Amnon Heiman
Metrics families (e.g., all metrics with the same name but with different labels) should have the same description.
The metric layer does not enforce that. Instead, it will use the first description provided.
It's a minor issue but the results are different than what you expect.

No need to backport.

Closes scylladb/scylladb#19947

* github.com:scylladb/scylladb:
  service/storage_proxy.cc  All metric groups should have the same description
  raft/server.cc: All metric groups should have the same description
2024-11-17 16:49:57 +02:00
Avi Kivity
9720bb1e5f partition_snapshot_row_cursor.hh: switch from boost ranges to std ranges
Converge on one range solution.
2024-11-15 14:39:39 +02:00
Avi Kivity
1c26c8deeb mutation: mutation_partition_v2.hh: switch from boost ranges to std ranges
Consolidate on one range solution. Fallout in mutation_partition_v2.cc
and row_cache_test.cc due to interoperability problems is adjusted.
2024-11-15 14:36:28 +02:00
Avi Kivity
de822d3a46 mutation: mutation_partition.hh: switch from boost ranges to std ranges
Consolidate on one range solution. Fallout in mutation_partition.cc
due to interoperability problems is adjusted.
2024-11-15 14:09:31 +02:00
Avi Kivity
6d110b530c partition_snapshot_reader.hh: drop unused include boost/range/algorithm/heap_algorithm.hpp 2024-11-15 14:02:19 +02:00
Kefu Chai
5bc03da0c4 tools/scylla-nodetool: rename estimated_row_count to estimated_partition_count
Rename the helper function from `estimated_row_count()` to `estimated_partition_count()`
to better reflect its actual behavior. While the underlying API endpoint is
"/column_family/metrics/estimated_row_count", it actually returns the estimated
partition count of the given table.

This follows up on 26ac2c23ef which updated server-side variable names but
did not change the API endpoint name. A separate change will update the tool's
documentation to address scylladb/scylladb#21586 specifically.

Refs scylladb/scylladb#21586
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#21597
2024-11-15 09:43:00 +02:00
Yaron Kaikov
8414cd743e .github/scripts/auto-backport.py: validate backport candidate with Fixes prefix
Adding `Fixes` validation to a PR when backport labels were added. When the auto backport process triggers (after promotion), we will ensure each PR with backport/x.y label also has in the PR body a `Fixes` reference to an issue

Adding also this validation to `pull_github_pr.sh` per @denesb request,

Fixes: https://github.com/scylladb/scylladb/issues/20021

Closes scylladb/scylladb#21563
2024-11-15 06:51:02 +02:00
Kefu Chai
4cc9d78801 compaction: document compaction::make_interposer_consumer()
for better maintainability

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#14982
2024-11-15 06:44:52 +02:00
Botond Dénes
fed2c6ba83 sstables/mx/reader: release column value buffer after consumed
data_consume_rows_context_m has a _column_value buffer it uses to read
key and column values into, preparing for parsing and consuming them.
This buffer is reset (released) in a few different cases:
* When using it for key - after consuming its content
* When using it for column value - when a colum has no value

However, the buffer is not released when used for a column value and the
column is consumed. This means that if a large column is read from the
sstable, this buffer can potentially linger and keep consuming memory
until either one of the other release scenarios is hit, or the reader is
destroyed.
Add a third release scenario, releasing the buffer after the row end was
consumed. This allows the buffer to be re-used between columns of the
same row, at the same time ensuring that a large buffer will not linger.

This patch can almost halve the memory consumption of reads in certain
circumstances. Point in case: the test
test_reader_concurrency_semaphore_memory_limit_engages starts to fail
after this fix, because the read doesn't trigger the OOM limit anymore
and needs doubling of the concurrency to keep passing.

This issue was found in a dtest
(`test_ics_refresh_with_big_sstable_files`), which writes some large
cells of up to 7MiB. After reading the row containing this large cell,
the reader holds on to the 7MiB buffer causing the semaphore's OOM
protection to kick in down the line.

Fixes: https://github.com/scylladb/scylladb/issues/21160

Closes scylladb/scylladb#21132
2024-11-14 17:24:53 +01:00
Kefu Chai
00810e6a01 treewide: include seastar/core/format.hh instead of seastar/core/print.hh
The later includes the former and in addition to `seastar::format()`,
`print.hh` also provides helpers like `seastar::fprint()` and
`seastar::print()`, which are deprecated and not used by scylladb.

Previously, we include `seastar/core/print.hh` for using
`seastar::format()`. and in seastar 5b04939e, we extracted
`seastar::format()` into `seastar/core/format.hh`. this allows us
to include a much smaller header.

In this change, we just include `seastar/core/format.hh` in place of
`seastar/core/print.hh`.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes scylladb/scylladb#21574
2024-11-14 17:45:07 +02:00
Michael Pedersen
309f1606ae docs: correct the storage size for n2-highmem-32 to 9000GB
updated storage size for n2-highmem-32 to 9000GB as this is default in SC

Closes scylladb/scylladb#21537
2024-11-14 17:16:44 +03:00
Pavel Emelyanov
298602b32d Merge 'message: do not include unused headers' from Kefu Chai
these unused includes are identified by clang-include-cleaner. after auditing the source files, all of the reports have been confirmed.

also, update the workflow to prevent future regressions of including unused headers in this subdirectory.

---

it's a cleanup, hence no need to backport.

Closes scylladb/scylladb#21560

* github.com:scylladb/scylladb:
  .github: add "message" to CLEANER_DIR
  message: do not include unused headers
2024-11-14 17:15:16 +03:00