in order to prevent future inclusion of unused headers, let's include
"message" subdirectory to CLEANER_DIR, so that this workflow can
identify the regressions in future.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
these unused includes are identified by clang-include-cleaner. after
auditing the source files, all of the reports have been confirmed.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
The req_param class is used to help parsing http request parameters from
strings into exact types (typically some simple types like strings,
integrals or boolean). On it there are three fields:
- name -- the parameter name
- param -- the parameter string value
- value -- the parameter value of desired type
The `param` thing is not really needed, it's only used by few places
that print it into logs, but they may as well just print the `value`
thing itself.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#21502
After calling api::parse_tables() the resulting vector of table names
cannot be empty, because in case parameter is missing, the parse_tables
function returns all tables from keyspace anyway.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#21501
Split compaction divides the partitions in an existing sstable into two groups and writes them into two new sstables, which replace the original one. The partition count from the original sstable is used as an estimate when writing the new ones, but this estimate is not accurate as the partitions are split between the two new sstables and each will contain only a portion of the original partition count. This also causes the bloom filters to be rebuilt at the end of compaction, as they were initially built with inaccurate estimates.
Fix this by using a better estimate for the output sstables, which is half the original partition count.
Fixes#20253
Improvement; No need to backport.
Closesscylladb/scylladb#20908
* github.com:scylladb/scylladb:
compaction: use better partition estimate for split compaction
compaction::table_state: implement `get_token_range_after_split()` wrapper
replica/table: implement `get_token_range_after_split()` wrappers
tablet_map: introduce `get_token_range_after_split()`
tablet_map: implement existing get_token_range() using the new variant
tablet_map: introduce `get_token_range()` variant
tablet_map: introduce `get_last_token()` variant
Add test to ensure backup tasks properly handle non-existent snapshots
by:
- Verifying backup task reports failure status
- Ensuring error is propagated through task status API
Previously untested edge case when backing up a snapshot that doesn't
exist in the test_backup.py tests.
Refs scylladb/scylladb#21381
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21385
When a backport PR is promoted to the release branch, we automatically close the backport PR (since GitHub will only close the one based on the default branch) and update the labels in the original PRs
In a situation when we have multiple `closes` prefixes, the script will use the first one (which is not the correct one), see 3ddb61c90e
Fixing this by always using the last line with the `closes` prefix
Closesscylladb/scylladb#21498
Split compaction divides the partitions in an existing sstable into two
groups and writes them into two new sstables, which replace the original
one. The partition count from the original sstable is used as an
estimate when writing the new ones, but this estimate is not accurate as
the partitions are split between the two new sstables and each will
contain only a portion of the original partition count. This also causes
the bloom filters to be rebuilt at the end of compaction, as they were
initially built with inaccurate estimates.
Fix this by using a better estimate for the output sstables based on the
token ranges written to them.
Fixes scylladb#20253
Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>
Expose the functionality of `tablet_map::get_token_range_after_split()`
via the replica::table class.
Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>
Added `get_token_range_after_split()`, which returns the token range the
given token will belong to after a tablet split. This is required to
estimate the token ranges of resultant sstables after a split.
Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>
Implement `get_token_range()` to return the token range of the specified
tablet with the given `log2_tablets` size. This will be used to deduce
which range a token will end up in if the tablet is split.
Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>
Implement `get_last_token()`, which returns the largest token owned
by the specified tablet with the given `log2_tablets` size. This will be
used to deduce token ranges for a tablet with any arbitrary
`tablet_count`.
Also, update the existing public `get_last_token()` to utilize the new
variant.
Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>
1. Add `retry_strategy` interface and default implementation for exponential back-off retry strategy.
2. Add new S3 related errors, also introduce additional errors to describe pure http errors that has no additional information in the body.
3. Add retries to the s3 client, all retries are coordinated by an instance of `retry_strategy`. In a case of error also parse response body in attempt to retrieve additional and more focused error information as suggested by AWS. See https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html. Introduce `aws_exception` to carry the original `aws_error`.
4. Discard whatever exception is thrown in `abort_upload` when aborting multipart upload since we don't care about cleanly aborting it since there are other means to clean up dangling parts, for example `rclone cleanup` or S3 bucket's Lifecycle Management Policy.
5. Add tests to cover retries, and retry exhaustion. Also add tests for jumbo upload.
6. Add the S3 proxy which is used to randomly inject retryable S3 errors to test the "retry" part of the S3 client. Switch the `s3_test` to use the S3 proxy. `s3_tests` set afloat `put_object` problem that was causing segmentation when retrying, fixed.
7. Extend the `s3_test` to use both `minio` and `proxy` configurations.
8. Add parameter to the proxy to seed the error injection randomization to make it replayable.
fixes: #20611fixes: #20613Closesscylladb/scylladb#21054
* github.com:scylladb/scylladb:
aws_errors: Make error messages more verbose.
test: Make the minio proxy randomization re-playable
test/boost/s3_test: add error injection scenarios to existing test suite
test: Switch `s3_test` to use proxy
test: Add more tests
client: Stop returning error on `DELETE` in multipart upload abortion
client: Fix sigsegv when retrying
client: Add retries
client: Adjust `map_s3_client_exception` to return exception instance
aws_errors: Change aws_error::parse to return std::optional<>
aws_errors: Add http errors mapping into aws_error
client: Add aws_exception mapping
aws_error: Add `aws_exeption` to carry original `aws_error`
aws_errors: Add new error codes
client: Introduce retry strategy
Add a buffer hint to the multishard reader. This is an internal hint, used by the multishard reader to provide a hint to the shard reader, on how much data exactly is needed by the multishard reader from the respective shard. This hint allows eliminating extraneous cross-shard round-trips and possible shard reader evict-recreate cycles. Building on this, repair sets its own row buffer size as the max buffer size on the multishard reader, ensuring that the row buffer is filled with the minimum amount of cross-shard round trips and minimal reader recreation.
To further eliminate unnecessary evictions, this PR also disables the multishard reader's read-ahead which is a mechanism that was designed to reduce latency for user-reads but it can be too aggressive for repair, causing unnecessary extra congestion on the already struggling streaming semaphores.
Refs: https://github.com/scylladb/scylladb/issues/18269
Fixes: https://github.com/scylladb/scylladb/issues/21113
The performance impact was measured with an SCT test, which creates a cluster of 3 nodes with 16 shards, then adds a 4th one with 12 shards.
Currently, it is the bootstrap time which is the worse in the case of mixed shard clusters, see below for the improvement measured during bootstrap:
| | master | buffer-hint | metric |
| ------------ | ------------- | ------------- | --------------------------------------------------- |
| evictions | 0.9M | 93.0K | scylla_database_paused_reads_permit_based_evictions |
| read (bytes) | 9.0T | 3.9T | scylla_reactor_aio_bytes_read |
| read (ops) | 88.0M | 33.5M | scylla_reactor_aio_reads |
| time | 56min | 20min | N/A |
This is a performance improvement, no backport required.
Closesscylladb/scylladb#20815
* github.com:scylladb/scylladb:
test/boost/mutation_reader_test: add test for multishard reader buffer hint
repair/row_level: disable read-ahead
db/config: introduce repair_multishard_reader_enable_read_ahead
readers/multishard: implement the read_ahead flag
replica/database: make_multishard_streaming_reader(): expose the read_ahead parameter
readers/multishard: add read_ahead parameter
repair/row_level: set max buffer size on multishard reader
replica/database: make_multishard_streaming_reader(): expose buffer_hint parameter
db/config: introduce enable_repair_multishard_reader_buffer_hint
readers/multishard: multishard_reader: pass hint to shard_reader
readers/multishard: shard_reader_v2::fill_reader_buffer(): respect the hint
readers/multishard: propagate fill_buffer_hint to shard_reader:fill_reader_buffer()
readers/multishard: shard_reader: extract buffer-fill into its own method
before this change, we specify the KillMode of the scylla-service
service unit explicitly to "process". according to
according to
https://www.freedesktop.org/software/systemd/man/latest/systemd.kill.html,
> If set to process, only the main process itself is killed (not recommended!).
and the document suggests use "control-group" over "process".
but scylla server is not a multi-process server, it is a multi-threaded
server. so it should not make any difference even if we switch to
the recommended "control-group".
in the light that we've been seeing "defunct" scylla process after
stopping the scylla service using systemd. we are wondering if we should
try to change the `KillMode` to "control-group", which is the default
value of this setting.
in this change, we just drop the setting so that the systemd stops the
service by stopping all processes in the control group of this unit
are stopped.
Refs scylladb/scylladb#21507
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21508
before this change, the header files generated with `idl-compiler.py`
are not regenerated if `idl-compiler.py` is updated. but they should,
as the change to the script could in turn change the generated header
files. because we have a typo in the `DEPENDS` argument,
`${idle_compiler}` is expanded to an empty string.
in this change, the typo is corrected, and the dependency from the
generated headers to the script is correctly reflected in the building
rules.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21475
In scylladb/scylladb#19745, view_builder was migrated to group0 and since then it is dependant on group0_service.
Because of this, group0_service should be initialized/destroyed before/after view_builder.
This patch also adds error injection to `raft_server_with_timeouts::read_barrier`, which does 1s sleep before doing the read barrier. There is a new test which reproduces the use after free bug using the error injection.
Fixesscylladb/scylladb#20772scylladb/scylladb#19745 is present in 6.2, so this fix should be backported to it.
Closesscylladb/scylladb#21471
* github.com:scylladb/scylladb:
test/boost/secondary_index_test: add test for use after free
api/raft: use `get_server_with_timeouts().read_barrier()` in coroutines
main,cql_test_env: start group0_service before view_builder
in seastar e96932b05f394b27cd0101e24f0584736795b50f, we stopped
including unused `fmt/ostream.h`. this helped to reduce the header
dependency. but this also broke the build of scylladb, as we rely
on the `fmt/ostream.h` indirectly included by seastar's header project.
in this change, we include `fmt/iostream.h` and `iostream` explictly
when we are using the declarations in them. this enables us to
- bump up the seastar submodule
- potentially reduce the header dependency as we will be able to
include seastar/core/format.hh instead of a more bloated
seastar/core/print.hh after bumping up seastar submodule
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21494
Reproduces scylladb/scylladb#20772.
Add error injection to `raft_server_with_timeouts::read_barrier`,
which does 1s sleep before doing the read barrier.
It is unsafe to do `get_server_with_timeouts().read_barrier()` in
continuations because `get_server_with_timeouts()` returns
raft server by value and it may be deallocated when `read_barrier()` yields,
causing use-after-return.
Simple workaround is to use the read barrier in coroutine and co_await
it. Then the raft server is kept on stack until the read barrier is
finished.
I've checked all codebase and it looks like the only place where
`group0_with_timeouts().read_barrier()` is in continuation, is
api/raft.cc.
Co-authored-by: Piotr Dulikowski <piodul@scylladb.com>
Separate the configuration for enabling the tablets feature from the enablement of tablets when creating new keyspaces.
This change always enables the TABLETS cluster feature and the tablets logic respectively.
The `enable_tablets` config option just controls whether tablets are enabled or disabled by default for new keyspaces.
If `enable_tablets` is set to `true`, tablets can be disabled using `CREATE KEYSPACE WITH tablets = { 'enabled': false }` as it is today.
If `enable_tablets` is set to `false`, tablets can be enabled using `CREATE KEYSPACE WITH tablets = { 'enabled': true }`.
The motivation for this change is to simplify the user experience of using tablets by setting the default for new keyspaces to false amd allowing the user to simply opt-in by using tablets = {enabled: true }.
This is not pissible today.
The user has to enable tablets by default for all new keyspaces (that use the NetworkTopologyStrategy) and then actively opt-out to use vnodes.
* Not required to be backported to OSS versions. May be backported to specific enterprise versions
* This PR resubmits https://github.com/scylladb/scylladb/pull/20729 that was reverted in 73b1f66b70 due to https://github.com/scylladb/scylladb/issues/21159 which is now fixed
Closesscylladb/scylladb#21451
* github.com:scylladb/scylladb:
data_dictionary: keyspace_metadata::describe: print tablets enabled also when defaulted
tablets_test: test enable/disable tablets when creating a new keyspace
treewide: always allow tablets keyspaces
feature_service: prevent enabling both tablets and gossip topology changes
alternator: create_keyspace_metadata: enable tablets using feature_service
For performance reasons, mutation_partition_v2::maybe_drop(), and by extension
also mutation_partition_v2::apply_monotonically(mutation_partition_v2&&)
can evict empty row entries, and hence change the continuity of the merged
entry.
For checking that apply_to_incomplete respects continuity,
test_apply_to_incomplete_respects_continuity obtains the continuity of
the partition entry before and after apply_to_incomplete by calling
e.squashed().get_continuity(). But squashed() uses apply_monotonically(),
so in some circumstances the result of squashed() can have smaller
continuity than the argument of squashed(), which messes with the thing
that the test is trying to check, and causes spurious failures.
This patch changes the method of calculating the continuity set,
so that it matches the entry exactly, fixing the test failures.
Fixesscylladb/scylladb#13757Closesscylladb/scylladb#21459
Provide a seed to the proxy randomization, the idea that the `test.py` will initialize the seed from `/dev/urandom` and print the seed when starting, in case some tests failed the dev is supposed to re-play it locally with the same seed (if it didnt repro otherwise) using the `start_s3_proxy.py` and providing it with the aforementioned seed using `--rnd-seed` command line argument
Add variants of existing S3 tests that route through a proxy instead of connecting directly to MinIO. The proxy allows injecting errors to validate error handling and recovery mechanisms under failure conditions.
Switch `s3_test` to use the S3 proxy which is used to randomly inject retryable S3 errors to test the "retry" part of the S3 client.
Fix `put_object` to make it retryable
Discard whatever exception is thrown in `abort_upload` when aborting multipart upload since we don't care about cleanly aborting it since there are other means to clean up dangling parts, for example `rclone cleanup` or S3 bucket's Lifecycle Management Policy
Add retries to the s3 client, all retries are coordinated by an instance of `retry_strategy`. In a case of error also parse response body in attempt to retrieve additional and more focused error information as suggested by AWS. See https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html.
Also move the expected http status check to the `make_s3_error_handler` since the http::client::make_request call is done with `nullopt` - we want to manage all the aws errors handling in s3 client to prevent the http client to validate it and fail before we have a chance to analyze the error properly
"Unfuturize" the `map_s3_client_exception` since the retryable client is going to be implemented using coroutines and no `future` is needed here, just to save unnecessary `co_await` on it
In scylladb/scylladb#19745, view_builder was migrated to group0
and since then it is dependent on group0_service.
Because of this, group0_service should be initialized/destroyed
before/after view_builder.
Fixesscylladb/scylladb#20772
Co-authored-by: Dawid Mędrek <dawid.medrek@scylladb.com>
Python and Python developers don't like directory names to include a minus sign, like "cql-pytest". In this patch we rename test/cql-pytest to test/cqlpy, and also change a few references in other code (e.g., code that used test/cql-pytest/run.py) and also references to this test suite in documentation and comments.
Arguably, the word "test" was always redundant in test/cql-pytest, and I want to leave the "py" in test/cqlpy to emphasize that it's Python-based tests, contrasting with test/cql which are CQL-request-only approval tests.
The second patch in the series fixes a small regression in the test/cqlpy/run script.
Fixes#20846
Test organization only, so backports not strictly necessary, but let's do them anyway because otherwise it will make any future backporting of tests in the cqlpy directory more messy than it needs to be.
Closesscylladb/scylladb#21446
* github.com:scylladb/scylladb:
test/cqlpy: fix "run" script without any parameters
test: rename "cql-pytest" to "cqlpy"
Test both configuration values for `enable_tablets`
and the possibility to explicitly enable or disable
tablets, respectively, when creating a keyspace using the
`tablets = {'enabled': true|false}` CREATE KEYSPACE option.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
With the tablets feature always enabled (Unless gossip toopology
changes are forced), the enable_tablets option now controls only
the default for newly created keyspaces.
Even when set to `false`, tablets are still enabled as a
feature and the user may explicitly enable tablets
using `CREATE KEYSPACE <name> WITH tablets = {'enabled': true}`
Note: best viewed with `git show -w`
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Tablets require raft consistent topology changes.
Therefore, document that they are incompatible in
the config help and prevent their usage in
`feature_config_from_db_config`
Fixesscylladb/scylladb#21075
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
