Commit Graph

44399 Commits

Author SHA1 Message Date
Botond Dénes
fdff4beb1f test/boost/reader_concurrency_semaphore_test: test the new diagnostics functionality
Adjust the test reader_concurrency_semaphore_dump_reader_diganostics to
also cover the new diagnostics functionality. The test is not a
correctness test -- the output has to be inspected by a human. But it is
good enough to make sure the code paths do not have any memory errors.
2024-09-12 08:31:25 -04:00
Botond Dénes
40b6616d3d reader_concurrency_semaphore: add bottleneck self-diagnosis to diagnosis dump
There are a few typical cases of bottlenecks, which can be easily
identified when dumping the semaphore diagnostics. Identify and print
these to fast-track investigations.
2024-09-12 08:31:25 -04:00
Botond Dénes
7d2b931619 reader_concurrency_semaphore: include trigger permit in diagnostic dump
In the previous patch, we provided an opportunity for callers to provide
a trigger permit, when calling `maybe_dump_reader_permit_diagnostics()`.
If the caller provided the trigger permit, include its details in the
dump, allowing the identification of the table and code-path of the
permit which triggered the dump.
2024-09-12 08:30:50 -04:00
Botond Dénes
c044904f07 reader_concurrency_semaphore: propagate permit to do_dump_reader_permit_diagnostics()
Will be used in the next patch.
2024-09-12 00:51:56 -04:00
Botond Dénes
67565a5eee reader_concurrency_semaphore: use consistent exception type for timeout
When a read times out, we use different exception types for the permit's
future (if the permit is waiting), or the permit's abort exception _ex
(which is used to abort ongoing reads). This patch changes both to use
named_semaphore_timed_out, which is the more verbose of the two.
2024-09-12 00:51:03 -04:00
Botond Dénes
036d27dc1b reader_concurrency_semaphore: dump diagnostics when non-waiting reader times out
Currently the semaphore only dumps diagnostics when a waiting reader
times out. The diagnostics are also useful when a non-waiting reader
(which is in the process of reading) times out, so also dump diagnostics
in this case.
Change the code to use a switch statement, so future addition of states
don't miss updating this logic.
2024-09-12 00:51:03 -04:00
Kefu Chai
3e84d43f93 treewide: use seastar::format() or fmt::format() explicitly
before this change, we rely on `using namespace seastar` to use
`seastar::format()` without qualifying the `format()` with its
namespace. this works fine until we changed the parameter type
of format string `seastar::format()` from `const char*` to
`fmt::format_string<...>`. this change practically invited
`seastar::format()` to the club of `std::format()` and `fmt::format()`,
where all members accept a templated parameter as its `fmt`
parameter. and `seastar::format()` is not the best candidate anymore.
despite that argument-dependent lookup (ADT for short) favors the
function which is in the same namespace as its parameter, but
`using namespace` makes `seastar::format()` more competitive,
so both `std::format()` and `seastar::format()` are considered
as the condidates.

that is what is happening scylladb in quite a few caller sites of
`format()`, hence ADT is not able to tell which function the winner
in the name lookup:

```
/__w/scylladb/scylladb/mutation/mutation_fragment_stream_validator.cc:265:12: error: call to 'format' is ambiguous
  265 |     return format("{} ({}.{} {})", _name_view, s.ks_name(), s.cf_name(), s.id());
      |            ^~~~~~
/usr/bin/../lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/format:4290:5: note: candidate function [with _Args = <const std::basic_string_view<char> &, const seastar::basic_sstring<char, unsigned int, 15> &, const seastar::basic_sstring<char, unsigned int, 15> &, const utils::tagged_uuid<table_id_tag> &>]
 4290 |     format(format_string<_Args...> __fmt, _Args&&... __args)
      |     ^
/__w/scylladb/scylladb/seastar/include/seastar/core/print.hh:143:1: note: candidate function [with A = <const std::basic_string_view<char> &, const seastar::basic_sstring<char, unsigned int, 15> &, const seastar::basic_sstring<char, unsigned int, 15> &, const utils::tagged_uuid<table_id_tag> &>]
  143 | format(fmt::format_string<A...> fmt, A&&... a) {
      | ^
```

in this change, we

change all `format()` to either `fmt::format()` or `seastar::format()`
with following rules:
- if the caller expects an `sstring` or `std::string_view`, change to
  `seastar::format()`
- if the caller expects an `std::string`, change to `fmt::format()`.
  because, `sstring::operator std::basic_string` would incur a deep
  copy.

we will need another change to enable scylladb to compile with the
latest seastar. namely, to pass the format string as a templated
parameter down to helper functions which format their parameters.
to miminize the scope of this change, let's include that change when
bumping up the seastar submodule. as that change will depend on
the seastar change.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
2024-09-11 23:21:40 +03:00
Pavel Emelyanov
f227f4332c test: Remove unused path local variable
Left after #20499 :(

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#20540
2024-09-11 23:10:25 +03:00
Avi Kivity
ed7d352e7d Merge 'Validate checksums for uncompressed SSTables' from Nikos Dragazis
This PR introduces a new file data source implementation for uncompressed SSTables that will be validating the checksum of each chunk that is being read. Unlike for compressed SSTables, checksum validation for uncompressed SSTables will be active for scrub/validate reads but not for normal user reads to ensure we will not have any performance regression.

It consists of:
* A new file data source for uncompressed SSTables.
* Integration of checksums into SSTable's shareable components. The validation code loads the component on demand and manages its lifecycle with shared pointers.
* A new `integrity_check` flag to enable the new file data source for uncompressed SSTables. The flag is currently enabled only through the validation path, i.e., it does not affect normal user reads.
* New scrub tests for both compressed and uncompressed SSTables, as well as improvements in the existing ones.
* A change in JSON response of `scylla validate-checksums` to report if an uncompressed SSTable cannot be validated due to lack of checksums (no `CRC.db` in `TOC.txt`).

Refs #19058.

New feature, no backport is needed.

Closes scylladb/scylladb#20207

* github.com:scylladb/scylladb:
  test: Add test to validate SSTables with no checksums
  tools: Fix typo in help message of scylla validate-checksums
  sstables: Allow validate_checksums() to report missing checksums
  test: Add test for concurrent scrub/validate operations
  test: Add scrub/validate tests for uncompressed SSTables
  test/lib: Add option to create uncompressed random schemas
  test: Add test for scrub/validate with file-level corruption
  test: Check validation errors in scrub tests
  sstables: Enable checksum validation for uncompressed SSTables
  sstables: Expose integrity option via crawling mutation readers
  sstables: Expose integrity option via data_consume_rows()
  sstables: Add option for integrity check in data streams
  sstables: Remove unused variable
  sstables: Add checksum in the SSTable components
  sstables: Introduce checksummed file data source implementation
  sstables: Replace assert with on_internal_error
2024-09-11 23:09:45 +03:00
Calle Wilund
b7839ec5d0 cql_test_env: Use temp socket + retry to ensure usable port for message_service if listen is enabled
Fixes #20543

In cql_test_env, if cfg_in.ms_listen is set, we try to get a free port for the current test on
which message service rpc can bind. This to allow multiple tests in parallel.

However, we just do this by using random and getting a number, not actually verifying it against
host ports in use.

This is complicated further by the fact that port reuse is effectively disabled in seastar
(see reactor::posix_reuseport_detect()). Due to this, the solution applied here is a combo
of
* Create temp socket with port = 0 to get a previously free port
* Close socket right before listen (to handle reuse not working)
* Retry on EADDRINUSE

Closes scylladb/scylladb#20547
2024-09-11 23:02:41 +03:00
Aleksandra Martyniuk
31ea74b96e db: system_keyspace: change version of topology_requests schema
In 880058073b a new column (request_type)
was added to topology_requests table, but the table's schema version
wasn't changed. Due to that during cluster upgrade, the old and the new
versions occur but they are not distinguishable.

Add offset to schema version of topology_requests table if it contains
request_type column.

Fixes: #20299.

Closes scylladb/scylladb#20402
2024-09-11 16:36:35 +03:00
Piotr Dulikowski
d98708013c Merge 'view: move view_build_status to group0' from Michael Litvak
Migrate the `system_distributed.view_build_status` table to `system.view_build_status_v2`. The writes to the v2 table are done via raft group0 operations.

The new parameter `view_builder_version` stored in `scylla_local` indicates whether nodes should use the old or the new table.

New clusters use v2. Otherwise, the migration to v2 is initiated by the topology coordinator when the feature is enabled. It reads all the rows from the old table and writes them to the new table, and sets `view_builder_version` to v2. When the change is applied, all view_builder services are updated to write and read from the v2 table.

The old table `system_distributed.view_build_status` is set to read virtually from the new table in order to maintain compatibility.

When removing a node from the cluster, we remove its rows from the table atomically (fixes https://github.com/scylladb/scylladb/issues/11836). Also, during the migration, we remove all invalid rows.

Fixes scylladb/scylladb#15329

dtest https://github.com/scylladb/scylla-dtest/pull/4827

Closes scylladb/scylladb#19745

* github.com:scylladb/scylladb:
  view: test view_build_status table with node replace
  test/pylib: use view_build_status_v2 table in wait_for_view
  view_builder: common write view_build_status function
  view_builder: improve migration to v2 with intermediate phase
  view: delete node rows from view_build_status on node removal
  view: sanitize view_build_status during migration
  view: make old view_build_status table a virtual table
  replica: move streaming_reader_lifecycle_policy to header file
  view_builder: test view_build_status_v2
  storage_service: add view_build_status to raft snapshot
  view_builder: migration to v2
  db:system_keyspace: add view_builder_version to scylla_local
  view_builder: read view status from v2 table
  view_builder: introduce writing status mutations via raft
  view_builder: pass group0_client and qp to view_builder
  view_builder: extract sys_dist status operations to functions
  db:system_keyspace: add view_build_status_v2 table
2024-09-11 13:02:58 +02:00
Nikos Dragazis
d1152a200f test: Add test to validate SSTables with no checksums
In a previous patch we extended the return status of
`sstables::validate_checksums()` to report if an SSTable cannot be
validated due to a missing CRC component (i.e., CRC.db does not appear
in TOC.txt).

Add a test case for this.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 13:12:40 +03:00
Nikos Dragazis
1f275c71b1 tools: Fix typo in help message of scylla validate-checksums
Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 13:12:39 +03:00
Nikos Dragazis
5c0a7f706b sstables: Allow validate_checksums() to report missing checksums
Change the return type of `sstable::validate_checksums()` from binary
(valid/invalid) to a ternary (valid/invalid/no_checksums). The third
status represents uncompressed SSTables without a CRC component (no
entry for CRC.db in the TOC).

Also, change the JSON response of `sstable validate-checksums` to expose
the new status. Replace the boolean value for valid/invalid checksums
with an object that contains two boolean keys: one that indicates if the
SSTable has checksums, and one that indicates if the checksums are valid
or not. The second key is optional and appears only if the SSTable has
checksums.

Finally, update the documentation to reflect the changes in the API.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 13:12:39 +03:00
Nikos Dragazis
5a284f4a9d test: Add test for concurrent scrub/validate operations
Theoretically it is possible to launch more than one scrub instances
simultaneously. Since the checksum component is a shared resource,
accesses have to be synchronized.

Add a test that launches two scrub operations in validate mode and
ensures that the checksum component is loaded once, referenced by all
scrub instances via shared pointers, and deleted once the scrub
operations finish. Introduce an injection point to achieve concurrent
execution of scrubs.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 13:12:39 +03:00
Nikos Dragazis
e2353f3b3e test: Add scrub/validate tests for uncompressed SSTables
Currently the unit tests check scrub in validate mode against compressed
SSTables only. Mirror the tests for uncompressed SSTables as well.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 13:12:39 +03:00
Nikos Dragazis
2991b09c8e test/lib: Add option to create uncompressed random schemas
Extend the `random_schema_specification` to support creating both
compressed and uncompressed schemas.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 13:12:32 +03:00
Nikos Dragazis
4f56c587f6 test: Add test for scrub/validate with file-level corruption
Currently, we test scrub/validate only against a corrupted SSTable with
content-level corruption (out-of-order partition key).

Add a test for file-level corruption as well. This should trigger the
checksum check in the underlying compressed file data source
implementation.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:28:59 +03:00
Nikos Dragazis
cc10a5f287 test: Check validation errors in scrub tests
Scrub was extended in PR #11074 to report validation errors but the
unit tests were not updated.

Update the tests to check the validation errors reported by scrub.
Validation errors must be zero for valid SSTables and non-zero for
invalid SSTables.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:28:59 +03:00
Nikos Dragazis
719757fba9 sstables: Enable checksum validation for uncompressed SSTables
Extend the `sstable::validate()` to validate the checksums of
uncompressed SSTables. Given that this is already supported for
compressed SSTables, this allows us to provide consistent behavior
across any type of SSTable, be it either compressed or uncompressed.

The most prominent use case for this is scrub/validate, which is now
able to detect file-level corruption in uncompressed SSTables as
well.

Note that this change will not affect normal user reads which skip
checksum validation altogether.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:28:59 +03:00
Nikos Dragazis
716fc487fd sstables: Expose integrity option via crawling mutation readers
Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:28:59 +03:00
Nikos Dragazis
1d2dc9f2e1 sstables: Expose integrity option via data_consume_rows()
Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:28:59 +03:00
Nikos Dragazis
2feced32f7 sstables: Add option for integrity check in data streams
Add a new boolean parameter in `sstable::data_stream()` to
enable/disable integrity mechanisms in the underlying data streams.
Currently, this only affects uncompressed SSTables and it allows to
enable/disable checksum validation on each chunk. The validation happens
transparently via the checksummed data source implementation.

The reason we need this option is to allow differentiating the behavior
between normal user reads and scrub/validate reads. We would like to
enable scrub to verify checksums for uncompressed SSTables, while
leaving normal user reads unchanged for performance reasons (read
amplification due to round up of reads to chunk size and loading of the
CRC component).

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:27:54 +03:00
Nikos Dragazis
d5bd40ad2c sstables: Remove unused variable
Remove unused stream variable from `sstable::data_stream()`. This was
introduced in commit 47e07b787e but never used.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:27:54 +03:00
Nikos Dragazis
2575d20f41 sstables: Add checksum in the SSTable components
Uncompressed SSTables store their checksums in a separate CRC.db file.
Add this in the list of SSTable components.

Since this component is used only for validation, load the component
on-demand for validation tasks and delete it when all validation tasks
finish. In more detail:

- Make the checksum component shareable and weakly referencable.
  Also, add a constructor since it is no longer an aggregate.
- Use a weak pointer to store a non-owning reference in the components
  and a shared pointer to keep the object alive while validation runs.
  Once validation finishes, the component should be cleaned up
  automatically.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:27:38 +03:00
Nikos Dragazis
b7dfba4c18 sstables: Introduce checksummed file data source implementation
Introduce a new data source implementation for uncompressed SSTables.

This is just a thin wrapper for a raw data source that also performs
checksum validation for each chunk. This way we can have consistent
behavior for compressed and uncompressed SSTables.

Signed-off-by: Nikos Dragazis <nikolaos.dragazis@scylladb.com>
2024-09-11 12:26:18 +03:00
Botond Dénes
0e5b444777 Merge 'database::get_all_tables_flushed_at: fix return value' from Lakshmi Narayanan Sreethar
The `database::get_all_tables_flushed_at` method returns a variable
without setting the computed all_tables_flushed_at value. This causes
its caller, `maybe_flush_all_tables` to flush all the tables everytime
regardless of when they were last flushed. Fix this by returning
the computed value from `database::get_all_tables_flushed_at`.

Fixes #20301

Requires a backport to 6.0 and 6.1 as they have the same issue.

Closes scylladb/scylladb#20471

* github.com:scylladb/scylladb:
  cql-pytest: add test to verify compaction_flush_all_tables_before_major_seconds config
  database::get_all_tables_flushed_at: fix return value
2024-09-11 11:43:45 +03:00
Abhi
9b09439065 raft: Add descriptions for requested abort errors
Fixes: scylladb/scylladb#18902

Closes scylladb/scylladb#20291
2024-09-10 17:56:29 +02:00
Botond Dénes
de81388edb Merge 'commitlog: Handle oversized entries' from Calle Wilund
Refs #18161

Yet another approach to dealing with large commitlog submissions.

We handle oversize single mutation by adding yet another entry
typo: fragmented. In this case we only add a fragment (aha) of
the data that needs storing into each entry, along with metadata
to correlate and reconstruct the full entry on replay.

Because these fragmented entries are spread over N segments, we
also need to add references from the first segment in a chain
to the subsequent ones. These are released once we clear the
relevant cf_id count in the base.
                 *
This approach has the downside that due to how serialization etc
works w.r.t. mutations, we need to create an intermediate buffer
to hold the full serialized target entry. This is then incrementally
written into entries of < max_mutation_size, successively requesting
more segments.

On replay, when encountering a fragment chain, the fragment is
added to a "state", i.e. a mapping of currently processing
frag chains. Once we've found all fragments and concatenated
the buffers into a single fragmented one, we can issue a
replay callback as usual.

Note that a replay caller will need to create and provide such
a state object. Old signature replay function remains for tests
and such.

This approach bumps the file format (docs to come).

To ensure "atomicity" we both force synchronization, and should
the whole op fail, we restore segment state (rewinding), thus
discarding data all we wrote.

Closes scylladb/scylladb#19472

* github.com:scylladb/scylladb:
  commitlog/database: Make some commitlog options updatable + add feature listener
  features/config: Add feature for fragmented commitlog entries
  docs: Add entry on commitlog file format v4
  commitlog_test: Add more oversized cases
  commitlog_replayer: Replay segments in order created
  commitlog_replayer: Use replay state to support fragmented entries
  commitlog_replayer: coroutinize partly
  commitlog: Handle oversized entries
2024-09-10 17:15:46 +03:00
Pavel Emelyanov
b6f662417c table: Remove unused database& argument from take_snapshot() method
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#20496
2024-09-10 14:53:06 +03:00
Gleb Natapov
af83c5e53e group0: stop group0 before draining storage service during shutdown
Currently storage service is drained while group0 is still active. The
draining stops commitlogs, so after this point no more writes are
possible, but if group0 is still active it may try to apply commands
which will try to do writes and they will fail causing group0 state
machine errors. This is benign since we are shutting down anyway, but
better to fix shutdown order to keep logs clean.

Fixes scylladb/scylladb#19665
2024-09-10 13:15:56 +02:00
Lakshmi Narayanan Sreethar
a0f4fe3fc4 cql-pytest: add test to verify compaction_flush_all_tables_before_major_seconds config
Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>
2024-09-10 16:39:05 +05:30
Lakshmi Narayanan Sreethar
4ca720f0bd database::get_all_tables_flushed_at: fix return value
The `database::get_all_tables_flushed_at` method returns a variable
without setting the computed all_tables_flushed_at value. This causes
its caller, `maybe_flush_all_tables` to flush all the tables everytime
regardless of when they were last flushed. Fix this by returning
the computed value from `database::get_all_tables_flushed_at`.

Fixes #20301

Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>
2024-09-10 16:35:47 +05:30
Yaniv Michael Kaul
a4ff0aae47 HACKIGN.md: clarify the use of dbuild when running test.py
If you are using dbuild, that's where test.py needs to run.

Also, replace 'Docker image' with the more generic 'container' term.

Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

Closes scylladb/scylladb#20336
2024-09-10 13:40:45 +03:00
Botond Dénes
08f109724b docs/cql/ddl.rst: fix description of sstable_compression
ScyllaDB doesn't support custom compressors. The available compressors
are the only available ones, not the default ones.
Adjust the text to reflect this.

Closes scylladb/scylladb#20225
2024-09-10 13:39:24 +03:00
Pavel Emelyanov
cfa59ab73d test: Use single temp dir for sharded<sstables::test_env>
The test-env in question is mostly started in one-shard mode. Also there
are several boost tests that start sharded<> environment. In that case
instances on different shards live in different temp dirs. That's not
critical yet, but better to have single directory for the whole test.

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#20412
2024-09-10 11:25:04 +03:00
Artsiom Mishuta
f95c257a1e [test.py]: Fail test teardown in case of task leakage
In test.py every asyncio task spawned during the test must be finished before the next test, otherwise, tests might affect each other results.
The developers are responsible for writing asyncio code in a way that doesn’t leave task objects unfinished.
Test.py has a mechanism that helps test writers avoid such tasks. At the end of each test case, it verifies that the test did not produce/leave any tasks and sets an event object that fails the next test at the start if this is the case(issue https://github.com/scylladb/scylladb/issues/16472)
The problem with this was that breaking the next test was counterintuitive, and the logging for this situation was insufficient and unobvious.

notes:  Task.cancel() is not an option to avoid task leakage
        1) Calling cancel() Does Not Cancel The Task :  the cancel() method just  request that the target task cancel.
        2) Calling cancel() Does Not Block Until The Task is Cancelled:  If the caller needs to know the task is cancelled and done, it could await for the target
        3) In particular PR, task.cancel() cancell task on client(ManagerClient) but not on http server(ScyllaManager). so "await" is needed.

Closes scylladb/scylladb#20012
2024-09-10 10:51:45 +03:00
Pavel Emelyanov
ac2127a640 test: Call table::make_sstable() directly in compaction test
The test in question generates a bunch of table_for_tests objects and
creates sstables for each. For that it calls test_env::make_sstable(),
but it can be made shorter, by calling table method directly.

The hidden goal of this change is to remove the explicit caller of
table::dir() method. The latter is going away.

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#20451
2024-09-10 10:19:20 +03:00
Botond Dénes
76bb22664a Merge 'Sanitize open_sstables() helper in compaction test' from Pavel Emelyanov
This includes
- coroutinization
- elimination of unused overload

Closes scylladb/scylladb#20456

* github.com:scylladb/scylladb:
  test: Squash two open_sstables() helper together
  test: Coroutinize open_sstables() helper
2024-09-10 10:18:33 +03:00
Botond Dénes
a4a4797e27 Merge 'Alternator: tests and other preparation towards allowing adding a GSI to an existing table' from Nadav Har'El
This series prepares us for working on #11567 -  allow adding a GSI to a pre-existing table. This will require changing the implementation of GSIs in Alternator to not use real columns in the schema for the materialized view, and instead of a computed column - a function which extracts the desired member from the `:attrs` map and de-serializes it.

This series does not contain the GSI re-implementation itself. Rather it contains a few small cleanups and mostly - new regression tests that cover this area, of adding and removing a GSI, and **using** a GSI, in more details than the tests we already had. I developed most of these tests while working on **buggy** fixes for #11567; The bugs in those implementations were exposed by the tests added here - they exposed bugs both in the new feature of adding or removing a GSI, and also regressions to the ordinary operation of GSI. So these tests should be helpful for whoever ends up fixing #11567, be it me based on my buggy implementation (which is _not_ included in this patch series), or someone else.

No backports needed - this is part of a new feature, which we don't usually backport.

Closes scylladb/scylladb#20383

* github.com:scylladb/scylladb:
  test/alternator: more extensive tests for GSI with two new key attributes
  test/alternator: test invalid key types for GSI
  test/alternator: test combination of LSI and GSI
  test/alternator: expand another test to use different write operations
  test/alternator: test GSIs with different key types
  alternator: better error message in some cases of key type mismatch
  test/alternator: test for more elaborate GSI updates
  test/alternator: strengthen tests for empty attribute values
  test/alternator: fix typo in test_batch.py
  test/alternator: more checks for GSI-key attribute validation
  Alternator: drop unneeded "IS NOT NULL" clauses in MV of GSI/LSI
  test/alternator: add more checks for adding/deleting a GSI
  test/alternator: ensure table deletions in test_gsi.py
2024-09-10 10:13:52 +03:00
Pavel Emelyanov
42f8d06a17 test: Use correct schema in directory tests with created table
There are some test cases in sstable_directory_test test actually create
a table with CQL and then try to manipulate its sstables with the help
of sstable_directory. Those tests use existing local helper that starts
sharded<sstable_directory> and this helper passes test-local static
schema to sstable_directory constructor. As a result -- the schema of a
table that test case created and the schema that sstable_directory works
with are different. They match in the columns layout, which helps the
test cases pass, but otherwise are two different schema objects with
different IDs. It's more correct to use table schema for those runs.

The fix introduces another helper to start sharded<sstable_directory>,
and the older wrapper around cql_test_env becomes unused. Drop it too
not to encourage future tests use it and re-introduce schema mismatch
again.

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#20499
2024-09-10 09:56:26 +03:00
Botond Dénes
fc690a60d8 Update tools/cqlsh submodule
* tools/cqlsh 86a280a1...b09bc793 (6):
  > build(deps): bump actions/download-artifact in /.github/workflows
  > cqlshlib/test: Add test_formatting.py
  > cqlshlib/test: Use assertEqual instead of assertEquals
  > cqlsh.py: Send DESCRIBE statement to server before parsing
  > cqlsh.py: Fix indentation
  > cqlsh.py: change shebang to /usr/bin/env python3
2024-09-10 08:11:40 +03:00
Lakshmi Narayanan Sreethar
2148e33d37 compaction: remove unnecessary share bump for split, scrub, and upgrade
When split, scrub, and upgrade compactions ran under the compaction
group, they had to bump up their shares to a minimum of 200 to prevent
slow progress as they neared completion, especially in workloads with
inconsistent ingestion rates. Since commit e86965c2 moved these
compactions to the maintenance group, this share bump is no longer
necessary. This patch removes the unnecessary share allocation.

Fixes #20224

Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>

Closes scylladb/scylladb#20495
2024-09-09 22:03:38 +03:00
Avi Kivity
9448260b30 Merge 'major compaction: check only sstables being compacted for tombstone garbage collection' from Lakshmi Narayanan Sreethar
Any expired tombstone can be garbage collected if it doesn't shadow data in the commit log, memtable, or uncompacting SSTables.

This PR introduces a new mode to major compaction, enabled by the `consider_only_existing_data` flag that bypasses these checks. When enabled, memtables and old commitlog segments are cleared with a system-wide flush and all the sstables (after flush) are included in the compaction, so that it works with all data generated up to a given time point.

This new mode works with the assumption that newly written data will not be shadowed by expired tombstones. So it ignores new sstables (and new data written to memtable) created after compaction started. Since there was a system wide flush, commitlog checks can also be skipped when garbage collecting tombstones. Introducing data shadowed by a tombstone during compaction can lead to undefined behavior, even without this PR, as the tombstone may or may not have already been garbage collected.

Fixes #19728

Closes scylladb/scylladb#20031

* github.com:scylladb/scylladb:
  cql-pytest: add test to verify consider_only_existing_data compaction option
  tools/scylla-nodetool: add consider-only-existing-data option to compact command
  api: compaction: add `consider_only_existing_data` option
  compaction: consider gc_check_only_compacting_sstables when deducing max purgeable timestamp
  compaction: do not check commitlog if gc_check_only_compacting_sstables is enabled
  tombstone_gc_state: introduce with_commitlog_check_disabled()
  compaction: introduce new option to check only compacting sstables for gc
  compaction: rename maybe_flush_all_tables to maybe_flush_commitlog
  compaction: maybe_flush_all_tables: add new force_flush param
2024-09-09 20:45:41 +03:00
Avi Kivity
894b85ce95 Merge 'hints: send hints with CL=ALL if target is leaving' from Piotr Dulikowski
Currently, when attempting to send a hint, we might choose its recipients in one of two ways:

- If the original destination is a natural endpoint of the hint, we only send the hint to that node and none other,
- Otherwise, we send the hint to all current replicas of the mutation.

There is a problem when we decommission a node: while data is streamed away from that node, it is still considered to be a natural endpoint of the data that it used to own. Because of that, it might happen that a hint is sent directly to it but streaming will miss it, effectively resulting in the hint being discarded.

As sending the hint _only_ to the leaving replica is a rather bad idea, send the hint to all replicas also in the case when the original destination of the hint is leaving.

Note that this is a conservative fix written only with the decommission + vnode-based keyspaces combo in mind. In general, such "data loss" can occur in other situations where the replica set is changing and we go through a streaming phase, i.e. other topology operations in case of vnodes and tablet load balancing. However, the consistency guarantees of hinted handoff in the face of topology changes are not defined and it is not clear what they should be, if there should be any at all. The picture is further complicated by the fact that hints are used by materialized views, and sending view updates to more replicas than necessary can introduce inconsistencies in the form of "ghost rows". This fix was developed in response to a failing test which checked the hint replay + decommission scenario, and it makes it work again.

Fixes scylladb/scylla-dtest#4582
Refs scylladb/scylladb#19835

Should be backported to 6.0 and 6.1; the dtest started failing due to topology on raft, which sped up execution of the test and exposed the preexisting problem.

Closes scylladb/scylladb#20488

* github.com:scylladb/scylladb:
  test: topology_custom/test_hints: consistency test for decommission
  test: topology_custom/test_hints: move sync point helpers to top level
  test: topology/util: extract find_server_by_host_id
  hints: send hints with CL=ALL if target is leaving
  hints: inline do_send_one_mutation
2024-09-09 18:23:13 +03:00
Avi Kivity
c3e19425bd Merge 'docs/dev/docker-hub.md: refresh aio-max-nr calculation' from Laszlo Ersek
~~~
What we have today in "docs/dev/docker-hub.md" on "aio-max-nr" dates back
to scylla commit f4412029f4 ("docs/docker-hub.md: add quickstart section
with --smp 1", 2020-09-22). Problems with the current language:

- The "65K" claim as default value on non-production systems is wrong;
  "fs/aio.c" in Linux initializes "aio_max_nr" to 0x10000, which is 64K.

- The section in question uses equal signs (=) incorrectly. The intent was
  probably to say "which means the same as", but that's not what equality
  means.

- In the same section, the relational operator "<" is bogus. The available
  AIO count must be at least as high (>=) as the requested AIO count.

- Clearer names should be used;
  adjust_max_networking_aio_io_control_blocks() in "src/core/reactor.cc"
  sets a great example:

  - "reactor::max_aio" should be called "storage_iocbs",

  - "detect_aio_poll" should be called "preempt_iocbs",

  - "reactor_backend_aio::max_polls" should be called "network_iocbs".

- The specific value 10000 for the last one ("network_iocbs") is not
  correct in scylla's context. It is correct as the Seastar default, but
  scylla has used 50000 since commit 2cfc517874 ("main, test: adjust
  number of networking iocbs", 2021-07-18).

Rewrite the section to address these problems.

See also:
- https://github.com/scylladb/scylladb/issues/5981
- https://github.com/scylladb/seastar/pull/2396
- https://github.com/scylladb/scylladb/pull/19921

Signed-off-by: Laszlo Ersek <laszlo.ersek@scylladb.com>
~~~

No need for backporting; the documentation being refreshed targets developers as audience, not end-users.

Closes scylladb/scylladb#20398

* github.com:scylladb/scylladb:
  docs/dev/docker-hub.md: refresh aio-max-nr calculation
  docs/dev/docker-hub.md: strip trailing whitespace
2024-09-09 15:04:38 +03:00
Botond Dénes
3e0bff161c Merge 'Use yielding directory lister in sstable_directory' from Pavel Emelyanov
The yielding lister is considered to be better replacement that scan_dir(lambda) one.
Also, the sstable directory will be patched to scan the contents of S3 bucket and yielding lister fits better for generalization.

Closes scylladb/scylladb#20114

* github.com:scylladb/scylladb:
  sstable_directory: Fix indentation after previous patches
  sstable_directory: Use yielding lister in .handle_sstables_pending_delete()
  sstable_directory: Use yielding lister in .cleanup_column_family_temp_sst_dirs()
  sstable_directory: Use yielding lister in .prepare()
  sstable_directory: Shorten lister loop
  sstable_directory: Use with_closeable() in .process()
  directory_lister: Add noexcept default move-constructor
2024-09-09 14:35:51 +03:00
Pavel Emelyanov
0f48847d02 test: Use shorter with_sstable_directory overload()
In sstable directory test there are two of those -- one that works on
path, state, env and callback, and the other one that just needs env and
callback, getting path from env and assuming state is normal.

Two test cases in this test can enjoy the shorter one.

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#20395
2024-09-09 14:25:24 +03:00
Pavel Emelyanov
2bfbbaffac test: Use sstables::test_env to make sstables for schema loader test
This test calls manager directly, but it's shorter to ask test_env for
that

Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>

Closes scylladb/scylladb#20431
2024-09-09 14:22:58 +03:00