Issue #8968 no longer exists when Raft-based schema updates are enabled
in Scylla (with --experimental-features=raft). Before we can close this
issue we need a way to re-run its test
test_keyspace.py::test_concurrent_create_and_drop_keyspace
with Raft and see it pass. But we also want the tests to continue to run
by default the older raft-less schema updates - so that this mode doesn't
regress during the potentially-long duration that it's still the default!
The solution in this patch is:
1. Introduce a "--raft" option to test/cql-pytest/run, which runs the tests
against a Scylla with the raft experimental feature, while the default is
still to run without it.
2. Introduce a text fixture "fails_without_raft" which marks a test which
is expected to fail with the old pre-raft code, but is expected to
pass in the new code.
3. Mark the test test_concurrent_create_and_drop_keyspace with this new
"fails_without_raft".
After this patch, running
test/cql-pytest/run --raft
test_keyspace.py::test_concurrent_create_and_drop_keyspace
Passes, which shows that issue 8968 was fixed (in Raft mode) - so we can say:
Fixes#8968
Running the same test without "--raft" still xfails (an expected failure).
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220208162732.260888-1-nyh@scylladb.com>
The system.config virtual tables prints each configuration variable of
type T based on the JSON printer specified in the config_type_for<T>
in db/config.cc.
For two variable types - experimental_features and tri_mode_restriction,
the specified converter was wrong: We used value_to_json<string> or
value_to_json<vector<string>> on something which was *not* a string.
Unfortunately, value_to_json silently casted the given objects into
strings, and the result was garbage: For example as noted in #10047,
for experimental_features instead of printing a list of features *names*,
e.g., "raft", we got a bizarre list of one-byte strings with each feature's
number (which isn't documented or even guaranteed to not change) as well
as carriage-return characters (!?).
So solution is a new printable_to_json<T> which works on a type T that
can be printed with operator<< - as in fact the above two types can -
and the type is converted into a string or vector of strings using this
operator<<, not a cast.
Also added a cql-pytest test for reading system.config and in particular
options of the above two types - checking that they contain sensible
strings and not "garbage" like before this patch.
Fixes#10047.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220209090421.298849-1-nyh@scylladb.com>
* seastar 0d250d15a...d27bf8b5a (5):
> Merge "Clean internal namespace in io_queue.cc" from Pavel E
> Making par.._for_each and max_conc.._for_each compatible with move-only views (like generators)
> tests: Perf test for smp::submit_to efficiency
> Merge "Auto-increase IO latency goal from reactor" from Pavel E
> reactor: Fix default task-quota-ms to be 0.5ms
If version is absent in cache, it will be fetched from the
coordinator. This is not expensive, but if the version is not known,
it must be also "synced". It means that the node will do a full schema
pull from the coordinator. This pull is expensive and can take seconds.
If the coordinator we pull from is at an old version, the pull will do
nothing and current node will soon forget the old version, initiating
another pull.
If some nodes stay at an old version for a long time for some reason,
this will make new coordinators initiate pulls frequently.
Increase the expiration period to 15 minutes to reduce the impact in
such scenarios.
Fixes#10042.
Message-Id: <20220207122317.674241-1-tgrabiec@scylladb.com>
Make only the first node in group0 to start as voter. Subsequent nodes
start as non-voters and request change to voter once bootstrap is
successful.
Add support for this in raft and a couple of minor fixes.
* alejo/raft-join-non-voting-v6:
raft: nodes joining as non-voters
raft: group 0: use cfg.contains() for config check
raft: modify_config: support voting state change
raft: minor: fix log format string
With trigger_compaction() being called after each new sstable is added
to the set, we'll get quadratic behavior because strategies like
tiered will sort all the candidates before iterating on them, so
complexity is ~ ((N - 1) * N * logN).
Additionally, compaction may be inefficient as we're not waiting for
the sstable set to settle, so table may end up missing files that
would allow for more efficient jobs.
The latter isn't a big problem because we have reshape running in an
earlier phase, so data layout should satisfy the strategy almost.
Boot is not affected by these problems because it temporarily
disables auto compaction, so trigger_compaction() is a no-op for it.
So refresh remains as the only one affected.
Fixes#10046.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20220208151154.72606-1-raphaelsc@scylladb.com>
Except for the first node creating the group0, make other nodes join as
non-voters and make them voters after successful bootstrap.
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
Currently, most of the failures that occur during CQL reads or writes are reported using C++ exceptions. Although the seastar framework avoids most of the cost of unwinding by keeping exceptions in futures as `std::exception_ptr`s, the exceptions need to be inspected at various points for the purposes of accounting metrics or converting them to a CQL error response. Analyzing the value and type of an exception held by `std::exception_ptr`'s cannot be done without rethrowing the exception, and that can be very costly even if the exception is immediately caught. Because of that, exceptions are not a good fit for reporting failures which happen frequently during overload, especially if the CPU is the bottleneck.
This PR introduces facilities for reporting exceptions as values using the boost::outcome library. As a first step, the need to use exceptions for reporting timeouts was eliminated for regular and batch writes, and no exceptions are thrown between creation of a `mutation_write_timeout_exception` and its serialization as a CQL response in the `cql_server`.
The types and helpers introduced here can be reused in order to migrate more exceptions and exception paths in a similar fashion.
Results of `perf_simple_query --smp 1 --operations-per-shard 1000000`:
Master (00a9326ae7)
128789.53 tps ( 82.2 allocs/op, 12.2 tasks/op, 49245 insns/op)
This PR
127072.93 tps ( 82.2 allocs/op, 12.2 tasks/op, 49356 insns/op)
The new version seems to be slower by about 100 insns/op, fortunately not by much (about 0.2%).
Tests: unit(dev), unit(result_utils_test, debug)
Closes#10014
* github.com:scylladb/scylla:
cql_test_env: optimize handling result_message::exception
transport/server: handle exceptions from coordinator_result without throwing
transport/server: propagate coordinator_result to the error handling code
transport/server: unwrap the exception result_message in process_xyz_internal
query_processor: add exception-returning variants of execute_ methods
modification_statement: propagate failed result through result_message::exception
batch_statement: propagate failed result through result_message::exception
cql_statement: add `execute_without_checking_exception_message`
result_message: add result_message::exception
storage_proxy: change mutate_with_triggers to return future<result<>>
storage_proxy: add mutate_atomically_result
storage_proxy: return result<> from mutate_result
storage_proxy: return result<> from mutate_internal
storage_proxy: properly propagate future from mutate_begin to mutate_end
storage_proxy: handle exceptions as values in mutate_end
storage_proxy: let mutate_end take a future<result<>>
storage_proxy: resultify mutate_begin
storage_proxy: use result in the _ready future of write handlers
storage_proxy: introduce helpers for dealing with results
exceptions: add coordinator_exception_container and coordinator_result
utils: add result utils
utils: add exception_container
There will be nodes in non-voting state in configuration, so can_vote()
is not a good check. Use newer cfg.contains().
Signed-off-by: Alejo Sanchez <alejo.sanchez@scylladb.com>
The single_node_cql_env uses query_processor::execute_xyz family of
methods to perform operations. Due to previous commits in this series,
they allocate one more task than before - a continuation that converts
result_message::exception into an exceptional future. We can recover
that one task by using variants of those methods which do not perform a
conversion, and turn .finally() invocations into .then()s which perform
conversion manually.
At the point where `result_message` is converted to a
`cql_server::response`, now the result message is inspected and returned
as failed `result<>` if it contained an error.
For now, the failed `result<>` is thrown as exception in `process` and
`process_on_shard`, but that will change in the next commit.
Adds variants of the execute_prepared, execute_direct and execute_batch
which are allowed to return exceptions as `result_message::exception`.
Because the `result_message::exception` must be explicitly handled by
the receiver, new variants are introduced in order not to accidentally
ignore the exception, which would be very bad.
Modifies the modification_statement code so that is converts failed
`result<>` into a `result_message::exception` without involving the C++
exception runtime.
Modifies the batch_statement code so that is converts failed `result<>`
into a `result_message::exception` without involving the C++ exception
runtime.
Adds a new virtual method to the cql_statement with a wordy name. The
new method is a variant of `execute`, but it is allowed to return errors
via the `result_message::exception` object.
The reason for an additional method is that there are many places in the
code which call `execute` but do not check the result in any way.
Because ignoring an exception unintentionally is a very bad thing, the
new method needs to be explicitly implemented by statements which can
return a `result_message::exception`, and explicitly called in the code
which is prepared to handle a `result_message::exception`.
In order to propagate exceptions as values through the CQL layer with
minimal modifications to the interfaces, a new result_message type is
introduced: result_message::exception. Similarly to
result_message::bounce_to_shard, this is an internal type which is
supposed to be handled before being returned to the client.
Changes the interface of `mutate_with_triggers` so that it returns
`future<result<>>` instead of `future<>`. No intermediate
`mutate_with_triggers_result` method is introduced because all call
sites will be changed in this PR so that they properly handle failed
`result<>`s with exceptions-as-values.
Similarly to `mutate_result` introduced in the previous commit,
`mutate_atomically_result` is introduced which returns some exceptions
inside `result<>`. The pre-existing `mutate_atomically` keeps the same
interface but uses `mutate_atomically_result` internally, converting
failed `result<>` to exceptional future if needed.
In order to be able to propagate exceptions-as-values from storage_proxy
but without having to modify all call sites of `mutate`, an in-between
method `mutate_result` is introduced which returns some exceptions
inside `result<>`. Now, `mutate` just calls the latter and converts
those exceptions to exceptional future if needed.
Instead of stupidly rethrowing the exception in failed result<>, the
`storage_proxy::mutate_end` function now inspects it with a visitor,
which does not involve any rethrows. Moreover, mutate_end now also
returns a `future<result<>>` instead of just `future<>`.
Changes the `storage_proxy::mutate_end` method to accept a
`future<result<>>` instead of `future<>`.
For the time being, all call call sites of that method pass a future
which is either exceptional or contains a result<> with a value.
Moreover, in case of a failed result<>, mutate_end just rethrows the
exception. Both of these will change in the upcoming commits of this PR.
Changes the type of the _ready promise in
abstract_write_response_handler - a promise used by the coordinator
logic to wait until the write operation is complete - to keep a
`result<>` instead of `void`. Now, a timeout is signalled by setting the
promise to a value containing a `result<>` with a mutation write timeout
exception - previously it was signalled by setting the promise to an
exceptional value.
This is just a first step on a long road of throwless propagation of the
error to the cql_server - for now, a failed result is immediately
converted to an exceptional future in `storage_proxy::response_wait`.
Adds coordinator_exception_container which is a typedef over
exception_container and is meant to hold exceptions returned from the
coordinator code path. Currently, it can only hold mutation write
timeout exceptions, because only that kind of error will be returned by
value as a result of this PR. In the future, more exception types can be
added.
Adds coordinator_result which is a boost::outcome::result that uses
coordinator_exception_container as the error type.
Adds a number of utilities for working with boost::outcome::result
combined with exception_container. The utilities are meant to help with
migration of the existing code to use the boost::outcome::result:
- `exception_container_throw_policy` - a NoValuePolicy meant to be used
as a template parameter for the boost::outcome::result. It protects
the caller of `result::value()` and `result::error()` methods - if the
caller wishes to get a value but the result has an error
(exception_container in our case), the exception in the container will
be thrown instead. In case it's the other way around,
boost::outcome::bad_result_access is thrown.
- `result_parallel_for_each` - a version of `parallel_for_each` which is
aware of results and returns a failed result in case any of the
parallel invocations return a failed result.
- `result_into_future` - converts a result into a future. If the result
holds a value, converts it into make_ready_future; if it holds an
exception, the exception is returned as make_exception_future.
- `then_ok_result` takes a `future<T>` and converts it into
a `future<result<T>>`.
- `result_wrap` adapts a callable of type `T -> future<result<T>>` and
returns a callable of type `result<T> -> future<result<T>>`.
"
This is the continuation of 3e31126b (Brush up the initial tokens
generation
code). The replica::database is still used as the configuration
provider, and
two of those bits can be easily fixed.
"
tests: unit(dev)
* 'br-database-no-replacing-config' of https://github.com/xemul/scylla:
database: Move is_replacing() and get_replace_address() (back) into storage_service
bootstrapper: Get 'is-replacing' via argument too
bootstrapper: Get replace address via argument
DynamoDB allows an UpdateItem operation "REMOVE x.y" when a map x
exists in the item, but x.y doesn't - the removal silently does
nothing. Alternator incorrectly generated an error in this case,
and unfortunately we didn't have a test for this case.
So in this patch we add the missing test (which fails on Alternator
before this patch - and passes on DynamoDB) and then fix the behavior.
After this patch, "REMOVE x.y" will remain an error if "x" doesn't
exist (saying "document paths not valid for this item"), but if "x"
exists and is a map, but "x.y" doesn't, the removal will silently
do nothing and will not be an error.
Fixes#10043.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220207133652.181994-1-nyh@scylladb.com>
1. There's nothing we can do about this error.
2. It doesn't affect any query
3. No need to reprort timeout errors here.
Refs #10029
Note that in 4.6.rc4-0.20220203.34d470967a0 (where the issue above was opened against)
the error is likely to be related to read_ahead failure which
is already reported as a warning in master since fc729a804b.
When backported, this patch should be applied after:
fc729a804bd7a993043d
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20220207080041.174934-1-bhalevy@scylladb.com>
Leaving group 0 in `decommission` would previously fail with RPC
exception because it happened after messaging service was shutdown.
Fixes#9845.
Message-Id: <20220201112743.9705-1-kbraun@scylladb.com>
As observed in #10026, after schema changes it somehow happened
that a column defition that does not match any of the base table
columns was passed to expression verification code.
The function that looks up the index of a column happens to return
-1 when it doesn't find anything, so using this returned index
without checking if it's nonnegative results in accessing invalid
vector data, and a segfault or silent memory corruption.
Therefore, an explicit check is added to see if the column was actually
found. This serves two purposes:
- avoiding segfaults/memory corruption
- making it easier to investigate the root cause of #10026Closes#10039
The CQL parser currently accepts a command like:
ALTER KEYSPACE ksname WITH replication = {
'class' : 'NetworkTopologyStrategy',
'dc1' : 2,
'dc1' : 3 }
But because these options are read into an std::map, one of the
definitions of 'dc1' is silently ignored (counter-intuitively, it is
the first setting which is kept, and the second setting is ignored.)
But this is most likely a user's typo, so a better choice is to report
this as a parse error instead of arbitrarly and silently keeping just
one of the settings.
This is what Cassandra does since version 3.11 (see
https://issues.apache.org/jira/browse/CASSANDRA-13369 and Cassandra
commit 1a83efe2047d0138725d5e102cc40774f3b14641), and this is what we do
in this patch.
The unit test cassandra_tests/validation/operations/alter_test.py::
testAlterKeyspaceWithMultipleInstancesOfSameDCThrowsSyntaxException,
translated from Cassandra's unit tests, now passes.
Fixes#10037.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220207113709.78613-1-nyh@scylladb.com>
Both helpers (natuarally) used to be storage-service methods, but then
were moved to databse because bootstrapper code wanted to know this info.
Now the bootstraper is equipped with necessary arguments.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This also removes the only usage of this helper outside of the storage
service. The place that needs it is the use_strict_sources_for_ranges()
checker and all the callers of it are aware of whether it's replacing
happenning or not.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The function cql3::util::maybe_quote() is used throughout Scylla to
convert identifier names (column names, table names, etc.) into strings
that can be embedded in CQL commands. maybe_quote() sometimes needs to
quote these identifier names, but when the identifier name is lowercase,
and not a CQL keyword, it is not quoted.
Not quoting identifier names when not needed is nice and pretty, but has
a forward-compatibility problem: If some CQL command with an unquoted
identifier is saved somewhere, and new version of Scylla adss this
identifier as a new reserved keyword - the CQL command will break.
So this patch introduces a new function, cql3::util::quote(), which
unconditionally quotes the given identifier.
The new function is not yet used in Scylla, but we add a unit test
(based on the test of maybe_quote()) to confirm it behaves correctly.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220118161217.231811-2-nyh@scylladb.com>
cql3::util::maybe_quote() is a utility function formatting an identifier
name (table name, column name, etc.) that needs to be embedded in a CQL
statement - and might require quoting if it contains non-alphanumeric
characters, uppercase characters, or a CQL keyword.
maybe_quote() made an effort to only quote the identifier name if neccessary,
e.g., a lowercase name usually does not need quoting. But lowercase names
that are CQL keywords - e.g., to or where - cannot be used as identifiers
without quoting. This can cause problems for code that wants to generate
CQL statements, such as the materialized-view problem in issue #9450 - where
a user had a column called "to" and wanted to create a materialized view
for it.
So in this patch we fix maybe_quote() to recognize invalid identifiers by
using the CQL parser, and quote them. This will quote reserved keywords,
but not so-called unreserved keywords, which *are* allowed as identifiers
and don't need quoting. This addition slows down maybe_quote(), but
maybe_quote() is anyway only used in heavy operations which need to
generate CQL.
This patch also adds two tests that reproduce the bug and verify its
fix:
1. Add to the low-level maybe_quote() test (a C++ unit test) also tests
that maybe_quote() quotes reserved keywords like "to", but doesn't
quote unreserved keywords like "int".
2. Add a test reproducing issue #9450 - creating a materialized view
whose key column is a keyword. This new test passes on Cassandra,
failed on Scylla before this patch, and passes after this patch.
It is worth noting that maybe_quote() now has a "forward compatiblity"
problem: If we save CQL statements generated by maybe_quote(), and a
future version introduces a new reserved keyword, the parser of the
future version may not be able to parse the saved CQL statement that
was generated with the old mayb_quote() and didn't quote what is now
a keyword. This problem can be solved in two ways:
1. Try hard not to introduced new reserved keywords. Instead, introduce
unreserved keywords. We've been doing this even before recognizing
this maybe_quote() future-compatibility problem.
2. In the next patch we will introduce quote() - which unconditionally
quotes identifier names, even if lowercase. These quoted names will
be uglier for lowercase names - but will be safe from future
introduction of new keywords. So we can consider switching some or
all uses of maybe_quote() to quote().
Fixes#9450
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220118161217.231811-1-nyh@scylladb.com>
This is a translation of Cassandra's CQL unit test source file
validation/operations/AlterTest.java into our our cql-pytest framework.
This test file includes 24 tests for various types of ALTER operations
(of keyspaces, tables and types). Two additional tests which required
multiple data centers to test were dropped with a comment explaining why.
All 24 tests pass on Cassandra, with 8 failing on Scylla reproducing
one already known Scylla issue and 5 previously-unknown ones:
Refs #8948: Cassandra 3.11.10 uses "class" instead of
"sstable_compression" for compression settings by default
Refs #9929: Cassandra added "USING TIMESTAMP" to "ALTER TABLE",
we didn't.
Refs #9930: Forbid re-adding static columns as regular and vice versa
Refs #9935: Scylla stores un-expanded compaction class name in system
tables.
Refs #10036: Reject empty options while altering a keyspace
Refs #10037: If there are multiple values for a key, CQL silently
chooses last value
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220206163820.1875410-2-nyh@scylladb.com>
Implement the nodetool.compact() function, requesting a major compaction
of the given table. As usual for the nodetool.* functions, this is
implemented with the REST API if available (i.e., testing Scylla), or
with the external "nodetool" command if not (for testing Cassandra).
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220206163820.1875410-1-nyh@scylladb.com>
Seastar uses POSIX IO for output in addition to C++ iostreams,
e.g. in print_safe(), where it write()s directly to stdout.
Instead of manipulating C++ output streams to reset
stdout/log files, reopen the underlying file descriptors
to output/log files.
Fixes#9962 "cql_repl prints junk into the log"
Message-Id: <20220204205032.1313150-1-kostja@scylladb.com>
Merged patch series by Konstantin Osipov:
Assorted fixes in test.py in preparation for cluster
testing:
- better logging
- async search for unit test cases
- ubuntu fixes
test.py: highlight the failure cause
test.py: clean up setting of scylla executable
test.py: speed up search for tests cases, use async
test.py: make case cache global
test.py: make --cpus option work on Ubuntu
test.py: create an own TestSuite instance for each path/mode combo
test.py: do not fail entire run if list-content fails due to ASAN
test.py: print subtest name on cancel
test.py: fix flake8 complaints
Adds `exception_container` - a helper type used to hold exceptions as a
value, without involving the std::exception_ptr.
The motivation behind this type is that it allows inspecting exception's
type and value without having to rethrow that exception and catch it,
unlike std::exception_ptr. In our current codebase, some exception
handling paths need to rethrow the exception multiple times in order to
account it into metrics or encode it as an error response to the CQL
client. Some types of exceptions can be thrown very frequently in case
of overload (e.g. timeouts) and inspecting those exceptions with
rethrows can make the overload even worse. For those kinds of exceptions
it is important to handle them as cheaply as possible, and
exception_container used with conjunction with boost::outcome::result
can help achieve that.
