in order to identify the problems caused by integer type promotion when comparing unsigned and signed integers, in this series, we
- address the warnings raised by `-Wsign-compare` compiler option
- add `-Wsign-compare` compiler option to the building systems
Closes#14652
* github.com:scylladb/scylladb:
treewide: use unsigned variable to compare with unsigned
treewide: compare signed and unsigned using std::cmp_*()
This series is based on top of the seastar relabel config API.
The series adds a REST API for the configuration, it allows to get and set it.
The API is registered under the V2 prefix and uses the swagger 2.0 definition.
After this series to get the current relabel-config configuration:
```
curl -X GET --header 'Accept: application/json' 'http://localhost:10000/v2/metrics-config/'
```
A set config example:
```
curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '[ \
{ \
"source_labels": [ \
"__name__" \
], \
"action": "replace", \
"target_label": "level", \
"replacement": "1", \
"regex": "io_que.*" \
} \
]' 'http://localhost:10000/v2/metrics-config/'
```
This is how it looks like in the UI
Closes#12670
* github.com:scylladb/scylladb:
api: Add the metrics API
api/config: make it optional if the config API is the first to register
api: Add the metrics.json Swagger file
Preparing for V2 API from files
this series addresses the FTBFS of tests with CMake, and also checks for the unknown parameters in `add_scylla_test()`
Closes#14650
* github.com:scylladb/scylladb:
build: cmake: build SEASTAR tests as SEASTAR tests
build: cmake: error out if found unknown keywords
build: cmake: link tests against necessary libraries
before this change, we sort sstables with compaction disabled, when we
are about to perform the compaction. but the idea of of guarding the
getting and registering as a transaction is to prevent other compaction
to mutate the sstables' state and cause the inconsistency.
but since the state is tracked on per-sstable basis, and is not related
to the order in which they are processed by a certain compaction task.
we don't need to guard the "sort()" with this mutual exclusive lock.
for better readability, and probably better performance, let's move the
sort out of the lock. and take this opportunity to use
`std::ranges::sort()` for more concise code.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14699
some times we initialize a loop variable like
auto i = 0;
or
int i = 0;
but since the type of `0` is `int`, what we get is a variable of
`int` type, but later we compare it with an unsigned number, if we
compile the source code with `-Werror=sign-compare` option, the
compiler would warn at seeing this. in general, this is a false
alarm, as we are not likely to have a wrong comparison result
here. but in order to prevent issues due to the integer promotion
for comparison in other places. and to prepare for enabling
`-Werror=sign-compare`. let's use unsigned to silence this warning.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
when comparing signed and unsigned numbers, the compiler promotes
the signed number to coomon type -- in this case, the unsigned type,
so they can be compared. but sometimes, it matters. and after the
promotion, the comparison yields the wrong result. this can be
manifested using a short sample like:
```
int main(int argc, char **argv) {
int x = -1;
unsigned y = 2;
fmt::print("{}\n", x < y);
return 0;
}
```
this error can be identified by `-Werror=sign-compare`, but before
enabling this compiling option. let's use `std::cmp_*()` to compare
them.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
This patch adds a metrics API implementation.
The API supports get and set the metric relabel config.
Seastar supports metrics relabeling in runtime, following Prometheus
relabel_config.
Based on metrics and label name, a user can add or remove labels,
disable a metric and set the skip_when_empty flag.
The metrics-config API support such configuration to be done using the
RestFull API.
As it's a new API it is placed under the V2 path.
After this patch the following API will be available
'http://localhost:10000/v2/metrics-config/' GET/POST.
For example:
To get the current config:
```
curl -X GET --header 'Accept: application/json' 'http://localhost:10000/v2/metrics-config/'
```
To set a config:
```
curl -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '[ \
{ \
"source_labels": [ \
"__name__" \
], \
"action": "replace", \
"target_label": "level", \
"replacement": "1", \
"regex": "io_que.*" \
} \
]' 'http://localhost:10000/v2/metrics-config/'
```
Until now, only the configuration API was part of the V2 API.
Now, when other APIs are added, it is possible that another API would be
the first to register. The first to register API is different in the
sense that it does not have a leading ',' to it.
This patch adds an option to mark the config API if it's the first.
This patch changes the base path of the V2 of the API to be '/'. That
means that the v2 prefix will be part of the path definition.
Currently, it only affect the config API that is created from code.
The motivation for the change is for Swagger definitions that are read
from a file. Currently, when using the swagger-ui with a doc path set
to http://localhost:10000/v2 and reading the Swagger from a file swagger
ui will concatenate the path and look for
http://localhost:10000/v2/v2/{path}
Instead, the base path is now '/' and the /v2 prefix will be added by
each endpoint definition.
From the user perspective, there is no change in current functionality.
Signed-off-by: Amnon Heiman <amnon@scylladb.com>
The `locator::topology::config::this_host_id` field is redundant
in all places that use `locator::topology::config`, so we can
safely remove it.
Closes#14638Closes#14723
With Raft-topology enabled, test_remove_garbage_group0_members has been
flaky when it should always fail. This has been discussed in #14614.
Disabling Raft-topology in the topology suite is problematic because
the initial cluster size is non-zero, so we have nodes that already use
Raft-topology at the beginning of the test. Therefore, we move
test_topology_remove_garbage_group0.py to the topology_custom suite.
Apart from disabling Raft-topology, we have to start 4 servers instead
of 1 because of the different initial cluster sizes.
Closes#14692
Fixes https://github.com/scylladb/scylladb/issues/13783
This commit documents the nodetool checkAndRepairCdcStreams
operation, which was missing from the docs.
The description is added in a new file and referenced from
the nodetool operations index.
Closes#14700
This is the first phase of providing strong exception safety guarantees by the generic `compaction_backlog_tracker::replace_sstables`.
Once all compaction strategies backlog trackers' replace_sstables provide strong exception safety guarantees (i.e. they may throw an exception but must revert on error any intermediate changes they made to restore the tracker to the pre-update state).
Once this series is merged and ICS replace_sstables is also made strongly exception safe (using infrastructure from size_tiered_backlog_tracker introduced here), `compaction_backlog_tracker::replace_sstables` may allow exceptions to propagate back to the caller rather than disabling the backlog tracker on errors.
Closes#14104
* github.com:scylladb/scylladb:
leveled_compaction_backlog_tracker: replace_sstables: provide strong exception safety guarantees
time_window_backlog_tracker: replace_sstables: provide strong exception safety guarantees
size_tiered_backlog_tracker: replace_sstables: provide strong exception safety guarantees
size_tiered_backlog_tracker: provide static calculate_sstables_backlog_contribution
size_tiered_backlog_tracker: make log4 helper static
size_tiered_backlog_tracker: define struct sstables_backlog_contribution
size_tiered_backlog_tracker: update_sstables: update total_bytes only if set changed
compaction_backlog_tracker: replace_sstables: pass old and new sstables vectors by ref
compaction_backlog_tracker: replace_sstables: add FIXME comments about strong exception safety
add fmt formatter for `utils::pretty_printed_data_size` and
`utils::pretty_printed_throughput`.
this is a part of a series to migrating from `operator<<(ostream&, ..)`
based formatting to fmtlib based formatting. the goal here is to enable
fmtlib to print `utils::pretty_printed_data_size` and
`utils::pretty_printed_throughput` without the help of `operator<<`.
please note, despite that it's more popular to use the IEC prefixes
when presenting the size of storage, i.e., MiB for 1024**2 bytes instead
of MB for 1000**2 bytes, we are still using the SI binary prefixes as
the default binary prefix, in order to preserve the existing behavior.
the operator<< for these types are removed.
the tests are updated accordingly.
Refs #13245Closes#14719
* github.com:scylladb/scylladb:
utils: drop operator<< for pretty printers
utils: add fmt formatter for pretty printers
these comments or docstrings are not in-sync with the code they
are supposed to explain. so let's update them accordingly.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14545
in this header, none of the exceptions defined by
`exceptions/exceptions.hh` is used. so let's drop the `#include`.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14718
`db/query_context.hh` contains the declaration of class
`db::query_context`. but `replica/table.cc` does not use or need
`db::query_context`.
so, in this change, the `#include` is removed.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14717
When running compaction task test on the same scylla instantion
other tests are run, some compaction tasks from other test cases may
be left in task manager. If they stay in memory long enough, they may
get unregistered during the compaction task test and cause bad_request
status.
Drain old compaction tasks before and after each test.
Fixes: #14584.
Closes#14585
since all callers of these operators have switched to fmt formatters.
let's drop them. the tests are updated accordingly.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
add fmt formatter for `utils::pretty_printed_data_size` and
`utils::pretty_printed_throughput`.
this is a part of a series to migrating from `operator<<(ostream&, ..)`
based formatting to fmtlib based formatting. the goal here is to enable
fmtlib to print `utils::pretty_printed_data_size` and
`utils::pretty_printed_throughput` without the help of `operator<<`.
please note, despite that it's more popular to use the IEC prefixes
when presenting the size of storage, i.e., MiB for 1024**2 bytes instead
of MB for 1000**2 bytes, we are still using the SI binary prefixes as
the default binary prefix, in order to preserve the existing behavior.
also, we use the singular form of "byte" when formating "1". this is
more correct.
the tests are updated accordingly.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Split long-runing database mutation tests.
At a trade-off with verbosity, split these sub-tests for the long running tests `database_with_data_in_sstables_is_a_mutation_source_`*.
Refs #13905Closes#14455
* github.com:scylladb/scylladb:
test/lib/mutation_source_test: bump ttl
test/boost/memtable_test: split memtable sub-tests
test/boost/database_test: split mutation sub-tests
it turns out we are creating two tables with the same name in
sstable_expired_data_ratio. and when creating the second table,
we don't destroy the first one.
this does not happen in the real world, we could tolerate this
in test. but this matters if we're going to have a system-wide per-table
registry which use the name of table as the table's identifier in the
registry. for instance, the metrics name for the tables would conflict.
so, in this series, we use different name for the tables under
testing. they can share the same set of sstables though. this fulfills
the needs of this test in question. also, we rename some variables
for better readability in this series.
Fixes https://github.com/scylladb/scylladb/issues/14657Closes#14665
* github.com:scylladb/scylladb:
test: rename variables with better names
test: use different table names in sstable_expired_data_ratio
test: explicitly capture variables
before this change, if the formatter size is greater than a pettabyte,
`exp` would be 6. but we still use it as the index to find the suffix
in `suffixes`, but the array's size is 6. so we would be referencing
random bits after "PB" for the suffix of the formatted size.
in this change
* loop in the suffix for better readability. and to avoid
the off-by-one errors.
* add tests for both pretty printers
Branches: 5.1,5.2,5.3
Fixes#14702
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14713
it is not supported by C++, and might not yield expected result.
as "0 <= d" evaluates to true, which is always less than "magic".
so let's avoid using it.
```
/home/kefu/dev/scylladb/test/raft/randomized_nemesis_test.cc:2908:23: error: result of comparison of constant 54313 with expression of type 'bool' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
2908 | assert(0 <= d < magic);
| ~~~~~~ ^ ~~~~~
```
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14695
prepare_expression() already validates the types and computes
the index of the field; no need to redo that work when
evaluating the expression.
The tests are adjusted to also prepare the expression.
Closes#14562
Use a large ttl (2h+) to avoid deletions for database_test.
An actual fix would be to make database_test to not ignore query_time,
but this is much harder.
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
Split long-runing memtable tests.
At a trade-off with verbosity, split these sub-tests for the long
running tests
test_memtable_with_many_versions_conforms_to_mutation_source*.
Refs #13905
Split long-runing database mutation tests.
At a trade-off with verbosity, split these sub-tests for the long
running tests database_with_data_in_sstables_is_a_mutation_source_*.
Refs #13905
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
we first use `cf` and then `lcs_table` later on in
`sstable_expired_data_ratio` to represent "tables_for_tests"
with schema of different compaction strategies.
to improve the readability, we rename the variables which are
related to STCS (Sized-Tiered Compaction Strategy) to "stcs_*", so
better reflect their relations, for better readability.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
it turns out we are creating two tables with the same name in
sstable_expired_data_ratio. and when creating the second table,
we don't destroy the first one.
this does not happen in the real world, we could tolerate this
in test. this matters if we're going to have a system-wide per-table
registry which use the name of table as the table's identifier in the
registry. for instance, the metrics name for the tables would conflict.
to avoid creating multiples tables with the same ${ks}.${cf},
after this change, we use different name for the tables under
testing, and they can share the same set of sstables though. this
fulfills the needs of this test in question, and the needs of
having per-table metrics with table id as their identifiers.
Fixes#14657
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Fixes https://github.com/scylladb/scylladb/issues/14490
This commit fixes mulitple links that were broken
after the documentation is published (but not in
the preview) due to incorrect syntax.
I've fixed the syntax to use the :docs: and :ref:
directive for pages and sections, respectively.
Closes#14664
Provide a way to fetch all pages for `run_async`.
While there, move the code to a common helper module.
Fixes https://github.com/scylladb/scylladb/issues/14451Closes#14688
* github.com:scylladb/scylladb:
test/pylib: handle paged results for async queries
test/pylib: move async query wrapper to common module
This is the last step of deprecation dance of DTCS.
In Scylla 5.1, users were warned that DTCS was deprecated.
In 5.2, altering or creation of tables with DTCS was forbidden.
5.3 branch was already created, so this is targetting 5.4.
Users that refused to move away from DTCS will have Scylla
falling back to the default strategy, either STCS or ICS.
See:
WARN 2023-07-14 09:49:11,857 [shard 0] schema_tables - Falling back to size-tiered compaction strategy after the problem: Unable to find compaction strategy class 'DateTieredCompactionStrategy
Then user can later switch to a supported strategy with
alter table.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Closes#14559
we intent to print the error message. but failed to pass it to the
formatter. if we actually run into this case, fmtlib would throw.
so in this change, we also print the error when
announcing schema change fails.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14623
This option allows user to change the number of ranges to stream in
batch per stream plan.
Currently, each stream plan streams 10% of the total ranges.
With more ranges per stream plan, it reduces the waiting time between
two stream plans. For example,
stream_plan1: shard0 (t0), shard1 (t1)
stream_plan2: shard0 (t2), shard1 (t3)
We start stream_plan2 after all shards finish streaming in stream_plan1.
If shard0 and shard1 in stream_plan1 finishes at different time. One of
the shards will be idle.
If we stream more ranges in a single stream plan, the waiting time will
be reduced.
Previously, we retry the stream plan if one of the stream plans is
failed. That's one of the reasons we want more stream plans. With RBNO
and 1f8b529e08 (range_streamer: Disable restream logic), the
restream factor is not important anymore.
Also, more ranges in a single stream plan will create bigger but fewer
sstables on the receiver side.
The default value is the same as before: 10% percentage of total ranges.
Fixes#14191Closes#14402
This is a followup to 1545ae2d3b
A new reader is introduced that automatically closes the underlying sstable reader once it's exhausted after a fast forward call.
Allowing us to revert 1fefe597e6 which is fragile.
Closes#14669
* github.com:scylladb/scylladb:
Revert "sstables: Close SSTable reader if index exhaustion is detected in fast forward call"
sstables: Automatically close exhausted SSTable readers in cleanup
Provide a flag to fetch all pages for run_async().
Add a simple test to random tables. Runs within 6 seconds in debug mode.
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
This reverts commit d3034e0fab.
The test modified by this commit
(view_build_test.test_view_update_generator_register_semaphore_unit_leak)
often fails, breaking build jobs.
The sharded<service>::invoke_on_all() has the ability to call method by
pointer with automagical unwrapping of sharded references. This makes
the code shorter.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closes#14684
Fixes https://github.com/scylladb/scylladb/issues/14598
This commit adds the description of minimum_keyspace_rf
to the CREATE KEYSPACE section of the docs.
(When we have the reference section for all ScyllaDB options,
an appropriate link should be added.)
This commit must be backported to branch-5.3, because
the feature is already on that branch.
Closes#14686
also, remove unnecessary forward declarations.
* compaction_manager_test_task_executor is only referenced
in the friend declaration. but this declaration does not need
a forward declaration of the friend class
* compaction_manager_test_task_executor is not used anywhere.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14680
Add `parameters` map to `injection_shared_data`. Now tests can attach
string data to injections that can be read in injected code via
`injection_handler`.
Closes#14521Closes#14608
* github.com:scylladb/scylladb:
tests: add a `parameters` argument to code that enables injections
api/error_injection: add passing injection's parameters to enable endpoint
tests: utils: error injection: add test for injection's parameters
utils: error injection: add a string-to-string map of injection's parameters
utils: error injection: rename received_messages_counter to injection_shared_data
before this change, we would have report in Jenkins like:
```
[Info] - 1 out of 3 times failed: failed.
== [File] - test/boost/commitlog_test.cc
== [Line] - 298
[Info] - passed: release=1, dev=1
== [File] - test/boost/commitlog_test.cc
== [Line] - 298
[Info] - failed: debug=1
== [File] - test/boost/commitlog_test.cc
== [Line] - 298
```
the first section is rendered from the an `Info` tag,
created by `test.py`. but the ending "failed" does not
help in this context, as we already understand it's failing.
so, in this change, it is dropped.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14546
