Return 400 Bad Request instead of 500 Internal Server Error
when user requests task or module which does not exist through
task manager and task manager test apis.
Closes#14166
* github.com:scylladb/scylladb:
test: add test checking response status when requested module does not exist
api: fix indentation
api: throw bad_param_exception when requested task/module does not exists
Compaction task test should only check the intended group of task.
Thus, the tasks are filtered in each test.
In order to be able to run the tests in parallel, checks for the tasks
of the same type are grouped together.
Fixes: #14131.
Closes#14161
* github.com:scylladb/scylladb:
test: put compaction task checks of the same type together
test: filter tasks of given compaction type
* seastar afe39231...99d28ff0 (16):
> file/util: Include seastar.hh
> http/exception: Use http::reply explicitly
> http/client: Include lost condition-variable.hh
> util: file: drop unnecessary include of reactor.hh
> tests: perf: add a markdown printer
> http/client: Introduce unexpected_status_error for client requests
> sharded: avoid #include <seastar/core/reactor.hh> for run_in_background()
> code: Use std::is_invocable_r_v instead of InvokeReturns
> http/client: Add ability to change pool size on the fly
> http/client: Add getters for active/idle connections counts
> http/client: Count and limit the number of connections
> http/client: Add connection->client RAII backref
> build: use the user-specified compiler when building DPDK
> build: use proper toolchain based on specified compiler
> build: only pass CMAKE_C_COMPILER when building ingredients
> build: use specified compiler when building liburing
Two changes are folded into the commit:
1. missing seastar/core/coroutine.hh include in one .cc file that
got it indirectly included before seastar reactor.hh drop from
file.hh
2. http client now returns unexpected_status_error instead of
std::runtime_error, so s3 test is updated respectively
Closes#14168
There are some cases where we can deduce that the entry was committed,
but we were throwing `commit_status_unknown`. Handle one more such case.
The added comment explains it in detail.
Also add a FIXME for another case where we throw `commit_status_unknown`
but we could do better.
Fixes: #14029Fixes: #14072Closes#14167
* github.com:scylladb/scylladb:
raft: server: throw fewer `commit_status_unknown`s from `wait_for_entry`
raft: replication test: don't hang if `_seen` overshots `_apply_entries`
raft: replication test: print a warning when handling `commit_status_unknown`
As in the previous commit, if a command gets doubly applied due to
`commit_status_unknown`, this will could lead to hard-to-debug failures;
one of them was the test hanging because we would never call
`_done.set_value()` in `state_machine::apply` due to `_seen`
overshooting `_apply_entries`.
Fix the problem and print a warning if we apply too many commands.
Fixes: #14072
`commit_status_unknown` may lead to double application and then a
hard-to-debug failure. But some tests actually rely on retrying it, so
print a warning and leave a FIXME for maybe a better future solution.
Ref: #14029
In task manager and task manager test rest apis when a task or module
which does not exist is requested, we get Internal Server Error.
In such cases, wrap thrown exceptions in bad_param_exception
to respond with Bad Request code.
Modify test accordingly.
statement_restrictions: forbid IS NOT NULL on columns outside the primary key
IS NOT NULL is currently allowed only when creating materialized views.
It's used to convey that the view will not include any rows that would make the view's primary key columns NULL.
Generally materialized views allow to place restrictions on the primary key columns, but restrictions on the regular columns are forbidden. The exception was IS NOT NULL - it was allowed to write regular_col IS NOT NULL. The problem is that this restriction isn't respected, it's just silently ignored (see #10365).
Supporting IS NOT NULL on regular columns seems to be as hard as supporting any other restrictions on regular columns.
It would be a big effort, and there are some reasons why we don't support them.
For now let's forbid such restrictions, it's better to fail than be wrong silently.
Throwing a hard error would be a breaking change.
To avoid breaking existing code the reaction to an invalid IS NOT NULL restrictions is controlled by the `strict_is_not_null_in_views` flag.
This flag can have the following values:
* `true` - strict checking. Having an `IS NOT NULL` restriction on a column that doesn't belong to the view's primary key causes an error to be thrown.
* `warn` - allow invalid `IS NOT NULL` restrictions, but throw a warning. The invalid restrictions are silently ignored.
* `false` - allow invalid `IS NOT NULL` restricitons, without any warnings or errors. The invalid restrictions are silently ignored.
The default values for this flag are `warn` in `db::config` and `true` in scylla.yaml.
This way the existing clusters will have `warn` by default, so they'll get a warning if they try to create such an invalid view.
New clusters with fresh scylla.yaml will have the flag set to `true`, as scylla.yaml overwrites the default value in `db::config`.
New clusters will throw a hard error for invalid views, but in older existing clusters it will just be a warning.
This way we can maintain backwards compatibility, but still move forward by rejecting invalid queries on new clusters.
Fixes: #10365Closes#13013
* github.com:scylladb/scylladb:
boost/restriction_test: test the strict_is_not_null_in_views flag
docs/cql/mv: columns outside of view's primary key can't be restricted
cql-pytest: enable test_is_not_null_forbidden_in_filter
statement_restrictions: forbid IS NOT NULL on columns outside the primary key
schema_altering_statement: return warnings from prepare_schema_mutations()
db/config: add strict_is_not_null_in_views config option
statement_restrictions: add get_not_null_columns()
test: remove invalid IS NOT NULL restrictions from tests
This is a regression introduced in f26179cd27.
Fixes: #14136
* 'gleb/set_group0' of github.com:scylladb/scylla-dev:
test: restart first node to see if it can boot after restart
service: move setting of group0 point in storage_service earlier
In test_compaction_task.py tests concerning the same type of compaction
are squashed together so that they are run synchronously and there is
no data race when the tests are run in parallel.
Add unit tests for the strict_is_not_null_in_views flag.
This flag controls the behavior in case of an invalid
IS NOT NULL restrictions on a materialized view column.
Materialized views allow only restricting columns
that belong to the view's primary key, all other
restrictions should be rejected.
There was a bug where IS NOT NULL restrictions
weren't rejected, but simply ignored instead.
This flags controls what should happen when the user
runs a query with such an invalid IS NOT NULL restriction.
strict_is_not_null_in_views can have the following values:
* `true` - strict checking, invalid queries are rejected
* `warn` - the query is allowed, but a warning is printed
* `false` - the query is allowed, the invalid restrictions
are silently ignored.
The tests are based on the ones for strict_allow_filtering,
which reside in the lines preceding the newly added tests.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
IS NOT NULL is now allowed only on the view's primary key columns,
so the xfail marker can be removed.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
IS NOT NULL shouldn't be allowed on columns
which are outside of the materialized view's primary key.
It's currently allowed to create views with such restrictions,
but they're silently ignored, it's a bug.
In the following commits restricting regular columns
with IS NOT NULL will be forbidden.
This is a breaking change.
Some users might have existing code that creates
views with such restrictions, we don't want to break it.
To deal with this a new feature flag is introduced:
strict_is_not_null_in_views.
By default it's set to `warn`. If a user tries to create
a view with such invalid restrictions they will get a warning
saying that this is invalid, but the query will still go through,
it's just a warning.
The default value in scylla.yaml will be `true`. This way new clusters
will have strict enforcement enabled and they'll throw errors when the
user tries to create such an invalid view,
Old clusters without the flag present in scylla.yaml will
have the flag set to warn, so they won't break on an update.
There's also the option to set the flag to `false`. It's dangerous,
as it silences information about a bug, but someone might want it
to silence the warnings for a moment.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
In that level no io_priority_class-es exist. Instead, all the IO happens
in the context of current sched-group. File API no longer accepts prio
class argument (and makes io_intent arg mandatory to impls).
So the change consists of
- removing all usage of io_priority_class
- patching file_impl's inheritants to updated API
- priority manager goes away altogether
- IO bandwidth update is performed on respective sched group
- tune-up scylla-gdb.py io_queues command
The first change is huge and was made semi-autimatically by:
- grep io_priority_class | default_priority_class
- remove all calls, found methods' args and class' fields
Patching file_impl-s is smaller, but also mechanical:
- replace io_priority_class& argument with io_intent* one
- pass intent to lower file (if applicatble)
Dropping the priority manager is:
- git-rm .cc and .hh
- sed out all the #include-s
- fix configure.py and cmakefile
The scylla-gdb.py update is a bit hairry -- it needs to use task queues
list for IO classes names and shares, but to detect it should it checks
for the "commitlog" group is present.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closes#13963
group0 pointer in storage_service should be set when group0 starts.
After f26179cd27 we start group0 earlier,
so we need to move setting of the group0 pointer as well.
as an alternative to passing the link-args using the environmental variable,
we can also use build script to pass the "-C link-args=<FLAG>" to the compiler.
see https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#cargorustc-link-argflag
to ensure that cargo is called again by ninja, after build.rs is
updated, build.rs is added as a dependency of {wasm} files along with
Cargo.lock.
this change is verified using following command
```
RUSTFLAGS='--print link-args' cargo build \
--target=wasm32-wasi \
--example=return_input \
--locked \
--manifest-path=Cargo.toml \
--target-dir=build/cmake/test/resource/wasm/rust
```
the output includes "-zstack-size=131072" in the argument passed to lld:
```
Compiling examples v0.0.0 (/home/kefu/dev/scylladb/test/resource/wasm/rust)
LC_ALL="C"
PATH="/usr/lib/rustlib/x86_64-unknown-linux-gnu/bin:/usr/lib/rustlib/x86_64-unknown-linux-gnu/bin/self-contained:/home/kefu/.local/bin:/home/kefu/bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin"
VSLANG="1033"
"lld"
"-flavor" "wasm" "--rsp-quoting=posix" "--export"
"_scylla_abi" "--export" "_scylla_free" "--export" "_scylla_malloc"
"--export" "return_input" "-z" "stack-size=1048576" "--stack-first"
"--allow-undefined" "--fatal-warnings" "--no-demangle"
...
"-L" "/usr/lib/rustlib/wasm32-wasi/lib"
"-L" "/usr/lib/rustlib/wasm32-wasi/lib/self-contained"
"-o"
"/home/kefu/dev/scylladb/build/cmake/test/resource/wasm/rust/wasm32-wasi/debug/examples/return_input-ef03083560989040.wasm"
"--gc-sections"
"--no-entry"
"-O0"
"-zstack-size=131072"
```
with this change, it'd be easier to build .wat files in CMake, so
we don't need to repeat the settings in both configure.py and
CMakeLists.txt
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14123
this series replaces hard-coded values with variables. will need to expand this test to cover most test cases when working on tiered-storage.
Closes#14137
* github.com:scylladb/scylladb:
s3/test: use variable for inserted data
s3/test: replace test_ks and test_cf with variables
s3/test: introduce format_tuples() for formatting CQL queries
instead of hardwiring the dataset in test, let's define them with
variables and use the variables instead.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
in order to make data set for testing more visible, format_tuples() is
introduced for formatting a dict into a set of structured values
consumable by CQL.
this function is added to test/cql-pytest/util.py in hope that it
can be reused by other tests using CQL.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
On main.cc, we have early commands which want to run prior to initialize
Seastar.
Currently, perf_fast_forward is breaking this, since it defined
"app_template app" on global variable.
To avoid that, we should defer running app_template's constructor in
scylla_fast_forward_main().
Fixes#13945Closes#14026
Summary of the patch set:
- eliminates not needed calls to rjson::find (~1% tps improvement in `perf-simple-query --write`)
- adds some very specific test in this area (more general cases were covered already)
- fixes some minor validation bug
Fixes https://github.com/scylladb/scylladb/issues/13251Closes#12675
* github.com:scylladb/scylladb:
alternator: fix unused ExpressionAttributeNames validation when used as a part of BatchGetItem
alternator: eliminate duplicated rjson::find() of ExpressionAttributeNames and ExpressionAttributeValues
The DeleteTable operation in Alternator shoudl return a TableDescription
object describing the table which has just been deleted, similar to what
DescribeTable returns
Fixes scylladb#11472
Closes#11628
we compile .wat files from .rs and .c source files since
6d89d718d9.
these .wat are used by test/cql-pytest/test_wasm.py . let's update
the CMake building system accordingly so these .wat files can also
be generated using the "wasm" target. since the ctest system is
not used. this change should allow us to perform this test manually.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14126
Don't block the thread which prevents concurrent tests from running
during this time. Use the dedicated `run_async`.
Also to silence `mypy` which complains that `manager.cql` is `Optional`
(so in theory might be `None`, e.g. after `driver_close`), use
`manager.get_cql()`.
Closes#14109
Secondary index creation is asynchronous, meaning it
takes time for existing data to be reflected within
the index. However, new data added after the
index is created should appear in it immediately.
The test consisted of two parts. The first created
a series of indexes for one table, added
test data to the table, and then ran a series of checks.
In the second part, several new indexes were added to
the same table, and checks were made to make sure that
already existing data would appear in them. This
last part was flaky.
The patch just moves the index creation statements
from the second part to the first.
Fixes: #14076Closes#14090
read_mutation_from_flat_mutation_reader might throw
so we need to close the reader returned from
ms.make_fragment_v1_stream also on the error
path to avoid the internal error abort when the
reader is destroyed while opened.
Fixes#14098
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Closes#14099
After c7826aa910, sstable runs are cleaned up together.
The procedure which executes cleanup was holding reference to all
input sstables, such that it could later retry the same cleanup
job on failure.
Turns out it was not taking into account that incremental compaction
will exhaust the input set incrementally.
Therefore cleanup is affected by the 100% space overhead.
To fix it, cleanup will now have the input set updated, by removing
the sstables that were already cleaned up. On failure, cleanup
will retry the same job with the remaining sstables that weren't
exhausted by incremental compaction.
New unit test reproduces the failure, and passes with the fix.
Fixes#14035.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Closes#14038
The command prints segment_manager address, because it's the manager
who's on interest, not the db::commitlog itself. Also it prints out all
found segments, it's just for convenience -- segments are in a vector of
shared pointers and it's handy to have object addresses instantly.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closes#14088
CWG 2631 (https://cplusplus.github.io/CWG/issues/2631.html) reports
an issue on how the default argument is evaluated. this problem is
more obvious when it comes to how `std::source_location::current()`
is evaluated as a default argument. but not all compilers have the
same behavior, see https://godbolt.org/z/PK865KdG4.
notebaly, clang-15 evaluates the default argument at the callee
site. so we need to check the capability of compiler and fall back
to the one defined by util/source_location-compat.hh if the compiler
suffers from CWG 2631. and clang-16 implemented CWG2631 in
https://reviews.llvm.org/D136554. But unfortunately, this change
was not backported to clang-15.
before switching over to clang-16, for using std::source_location::current()
as the default parameter and expect the behavior defined by CWG2631,
we have to use the compatible layer provided by Seastar. otherwise
we always end up having the source_location at the callee side, which
is not interesting under most circumstances.
so in this change, all places using the idiom of passing
std::source_location::current() as the default parameter are changed
to use seastar::compat::source_location::current(). despite that
we have `#include "seastarx.h"` for opening the seastar namespace,
to disambiguate the "namespace compat" defined somewhere in scylladb,
the fully qualified name of
`seastar::compat::source_location::current()` is used.
see also 09a3c63345, where we used
std::source_location as an alias of std::experimental::source_location
if it was available. but this does not apply to the settings of our
current toolchain, where we have GCC-12 and Clang-15.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14086
The `view_update_write_response_handler` class, which is a subclass of
`abstract_write_response_handler`, was created for a single purpose:
to make it possible to cancel a handler for a view update write,
which means we stop waiting for a response to the write, timing out
the handler immediately. This was done to solve issue with node
shutdown hanging because it was waiting for a view update to finish;
view updates were configured with 5 minute timeout. See #3966, #4028.
Now we're having a similar problem with hint updates causing shutdown
to hang in tests (#8079).
`view_update_write_response_handler` implements cancelling by adding
itself to an intrusive list which we then iterate over to timeout each
handler when we shutdown or when gossiper notifies `storage_proxy`
that a node is down.
To make it possible to reuse this algorithm for other handlers, move
the functionality into `abstract_write_response_handler`. We inherit
from `bi::list_base_hook` so it introduces small memory overhead to
each write handler (2 pointers) which was only present for view update
handlers before. But those handlers are already quite large, the
overhead is small compared to their size.
Use this new functionality to also cancel hint write handlers when we
shutdown. This fixes#8079.
Closes#14047
* github.com:scylladb/scylladb:
test: reproducer for hints manager shutdown hang
test: pylib: ScyllaCluster: generalize config type for `server_add`
test: pylib: scylla_cluster: add explicit timeout for graceful server stop
service: storage_proxy: make hint write handlers cancellable
service: storage_proxy: rename `view_update_handlers_list`
service: storage_proxy: make it possible to cancel all write handler types
This reverts commit 52e4edfd5e, reversing
changes made to d2d53fc1db. The associated test
fails with about 10% probablity, which blocks other work.
Fixes#13919Reopens#13747
This set encapsulates ks/cf directories creation and deletion into keyspace and table classes methods. This is needed to facilitate making the storage initialization storage-type aware in the future. Also this makes the replica/ code less involved in formatting sstables' directory path by hand.
refs: #13020
refs: #12707Closes#14048
* github.com:scylladb/scylladb:
keyspace: Introduce init_storage()
keyspace: Remove column_family_directory()
table: Introduce destroy_storage()
table: Simplify init_storage()
table: Coroutinize init_storage()
table: Relocate ks.make_directory_for_column_family()
distributed_loader: Use cf.dir() instead of ks.column_family_directory()
test: Don't create directory for system tables in cql_test_env
This reverts commit 3c54d5ec5e.
The reverted change fixed the FTBFS of the test in question with Clang 16,
which rightly stopped convert the LHS of `"hello" == sstring{"hello"}` to
the type of the type acceptable by the member operator even we have a
constructor for this conversion, like
class sstring {
public:
bar_t(const char*);
bool operator==(const sstring&) const;
bool operator!=(const sstring&) const;
};
because we have an operator!=, as per the draft of C++ standard
https://eel.is/c++draft/over.match.oper#4 :
> A non-template function or function template F named operator==
> is a rewrite target with first operand o unless a search for the
> name operator!= in the scope S from the instantiation context of
> the operator expression finds a function or function template
> that would correspond ([basic.scope.scope]) to F if its name were
> operator==, where S is the scope of the class type of o if F is a
> class member, and the namespace scope of which F is a member
> otherwise.
in 397f4b51c3, the seastar submodule was
updated. in which, we now have a dedicated overload for the `const char*`
case. so the compiler is now able to compile the expression like
`"hello" == sstring{"hello"}` in C++20 now.
so, in this change, the workaround is reverted.
Closes#14040
If server shutdown hangs, the `manager.server_stop_gracefully` call
would eventually (after 5 minutes) timeout with a cryptic
`TimeoutError`; it's a generic timeout for performing requests by the
tests to `ScyllaClusterManager`. It was non-obvious how to find what
actually caused the timeout - you'd have to browse multiple logs.
Introduce an explicit timeout in `ScyllaServer.stop_gracefully`. Set it
to 1 minute. Whether this is a good value may be arguable, but shutdown
taking longer than that probably indicates problems. The important thing
is that this timeout is shorter than the generic request timeout.
If this times out we get a nice error in the test:
```
E test.pylib.rest_client.HTTPError: HTTP error 500, uri: http+unix://api/cluster/server/1/stop_gracefully, params: None, json: None, body:
E Stopping server ScyllaServer(1, 127.162.40.1, 826d5884-4696-4a22-80a7-cc872aa43102) gracefully took longer than 60s
```
instead of using BOOST_REQUIRE() use, for instance
BOOST_REQUIRE_NE() and BOOST_REQUIRE_EQUAL() for better
error message when the test fails, as Boost::test would
print out the LHS and RHS of the comparison expression
if it fails.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closes#14050
BatchGetItem request is a map of table names and 'sub-requests', ExpressionAttributeNames is defined on
'sub-request' level but the code was instead checking the top level, obtaining nullptr every time which
effectively disables unused names check.
Fixes#13251
We've observed sporadic failures of this test in CI related to driver reconnection after server restart.
Fixes#14032Closes#14027
* github.com:scylladb/scylladb:
test: test_tablets.py: Wait for driver to see the hosts after restart
test: test_tablets.py: Pass server id to server_restart()
test: test_tablets.py: Add missing await on server_restart()
Apparently, the driver may be still establishing connections in the
background after connecting to the cluster and queries may fail with:
cassandra.cluster.NoHostAvailable
Replace reconnection with wait_for_cql_and_get_hosts(), which ensures
that the driver sees the host.
