With this change, ME sstables start carrying their originating host
id, which makes ME format feature-complete so it can be made default.
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
Later in this series the ME sstable format is made default, which
means that `system.local` will likely be written as ME.
Since, in ME, originating host id is a part of sstable stats metadata,
the local host id needs to either already be cached by the time
`system.local` is flushed, or to somehow be special-case-ignored when
flushing `system.local`.
The former (done here) is optimistic (cache before flush), but the
alternative would be an abstraction violation and would also cost a
little time upon each sstable write.
(Cache-before-flush could be undone by catching any exceptions during
flush and un-caching, but inability to `co_await` in catch clauses
makes the code look rather awkward. And there is no need to bother
because bootstrap failures should be fatal anyway)
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
The "populate_from_quarantine_works" test case creates sstables with
one db config, then reads them with another. Ensure that both configs
have the same host id so the sstables pass validation.
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
Since ME sstable format includes originating host id in stats
metadata, local host id needs to be made available for writing and
validation.
Both Scylla server (where local host id comes from the `system.local`
table) and unit tests (where it is fabricated) must be accomodated.
Regardless of how the host id is obtained, it is stored in the db
config instance and accessed through `sstables_manager`.
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
The class is already inherited from in tests (along with overriding a
non-virtual method), so this seems to be called for.
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
We want it to be cached before any sstable is written, so do it right
after system_keyspace::minimal_setup().
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
Initialize it to "md" until ME format support is
complete (i.e. storing originating host id in sstable stats metadata
is implemented), so at present there is no observable change by
default.
Also declare "enable_sstables_md_format" unused -- the idea, going
forward, being that only "sstable_format" controls the written sstable
file format and that no more per-format enablement config options
shall be added.
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
(that is, instances of `std::optional`).
The ME sstable format includes optional originating host id in stats
metadata. We know how to write and parse uuids, but not how to write
and parse optionals.
The format is (used by C* in this case, and also happens to be
consistent with how booleans are serialized): first a boolean
indicating whether the contents are present (0 or 1, as a byte), then
the contents (if any).
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
Prerequisite for the "ME sstable format support" series (which has been
posted to the mailing list) -- to be merged or rejected together with
that.
Signed-off-by: Michael Livshin <michael.livshin@scylladb.com>
Closes#9939
In commit a664ac7ba5, the Alternator
schema-modifying code (e.g., delete_table()) was reorganized to support
the new Raft-based schema modifications. Schema modifications now work
with an "optimistic locking" approach: We retrieve the current schema
version id ("group0_guard"), reads the current schema and verifies it
can do the changes it wants to do, and then does them with
mm.announce(group0_guard) - which will fail if the schema version is not
current because some other concurrent modification beat us in the race.
This means that we need to do this whole read-modify-write (group0_guard,
checking the schema, creating mutations, calling mm.announce()) in a
*retry loop*. We have such a loop in the CQL code but it's missing in the
Alternator code. In this patch we don't add the loop yet, but add FIXMEs
to remind us where it's missing.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220214154435.544125-1-nyh@scylladb.com>
We add reproducing tests for two known Alternator issues, #6391 and #9868,
which involve the non-atomicity of table creation. Creating a table
currently involves multiple steps - creating a keyspace, a table,
materialized views, and tags. If some of these steps succeed and some
fail, we get an InternalServerError and potentially leave behind some
half-built table.
Both issues will be solved by making better use of the new Raft-based
capabilities of making multiple modifications to the schema atomically,
but this patch doesn't fix the problem - it just proves it exist.
The new tests involve two threads - one repeatedly trying to create a
table with a GSI or with tags - and the other thread repeatedly trying
to delete the same table under its feet. Both bugs are reproduced almost
immediately.
Note that like all test/alternator tests, the new tests are usually run on
just one node. So when we fix the bug and these tests begin to pass,
it will not be a proof that concurrent schema modification works safely
on *different* nodes. To prove that, we will also need a multi-node test.
However, this test can prove that we used Raft-based schema modification
correctly - and if we assume that the Raft-based schema modification
feature is itself correct, then we can be sure that CreateTable will be
correct also across multiple nodes. Although it won't hurt to check it
directly.
Refs #6391
Refs #9868
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220207223100.207074-1-nyh@scylladb.com>
This reverts commit 4c05e5f966.
Moving cleanup to maintenance group made its operation time up to
10x slower than previous release. It's a blocker to 4.6 release,
so let's revert it until we figure this all out.
Probably this happens because maintenance group is fixed at a
relatively small constant, and cleanup may be incrementally
generating backlog for regular compaction, where the former is
fighting for resources against the latter.
Fixes#10060.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20220213184306.91585-1-raphaelsc@scylladb.com>
This reverts commit 23da2b5879. It causes
the node to quickly run out of memory when many schema changes are made
within a small time window.
Fixes#10071.
Adds `utils::result_try` and `utils::result_futurize_try` - functions which allow to convert existing try..catch blocks into a version which handles C++ exceptions, failed results with exception containers and, depending on the function variant, exceptional futures using the same exception handling logic.
For example, you can convert the following try..catch block:
try {
return a_function_that_may_throw();
} catch (const my_exception& ex) {
return 123;
} catch (...) {
throw;
}
...to this:
return utils::result_try([&] {
return a_function_that_may_throw_or_return_a_failed_result();
}, utils::result_catch<my_exception>([&] (const Ex&) {
return 123;
}), utils::result_catch_dots([&] (auto&& handle) {
return handle.into_result();
});
Similarly, `utils::result_futurize_try` can be used to migrate `then_wrapped` or `f.handle_exception()` constructs.
As an example of the usability of the new constructs, two places in the current code which need to simultaneously handle exceptions and failed results are converted to use `result_try` and `result_futurize_try`.
Results of `perf_simple_query --smp 1 --operations-per-shard 1000000 --write`:
```
127041.61 tps ( 67.2 allocs/op, 14.2 tasks/op, 52422 insns/op)
126958.60 tps ( 67.2 allocs/op, 14.2 tasks/op, 52409 insns/op)
127088.37 tps ( 67.2 allocs/op, 14.2 tasks/op, 52411 insns/op)
127560.84 tps ( 67.2 allocs/op, 14.2 tasks/op, 52424 insns/op)
127826.61 tps ( 67.2 allocs/op, 14.2 tasks/op, 52406 insns/op)
126801.02 tps ( 67.2 allocs/op, 14.2 tasks/op, 52420 insns/op)
125371.51 tps ( 67.2 allocs/op, 14.2 tasks/op, 52425 insns/op)
126498.51 tps ( 67.2 allocs/op, 14.2 tasks/op, 52427 insns/op)
126359.41 tps ( 67.2 allocs/op, 14.2 tasks/op, 52423 insns/op)
126298.27 tps ( 67.2 allocs/op, 14.2 tasks/op, 52410 insns/op)
```
The number of tasks and allocations is unchanged. The number of instructions per operations seems similar, it may have increased slightly (by 10-20) but it's hard to tell for sure because of the noisiness of the results.
Tests: unit(dev)
Closes#10045
* github.com:scylladb/scylla:
transport: use result_try in process_request_one
storage_proxy: use result_futurize_try in mutate_end
storage_proxy: temporarily throw exception from result in mutate_end
utils: add result_try and result_futurize_try
After the mechanical change in fcb8d040e8
("treewide: use Software Package Data Exchange (SPDX) license identifiers"),
a few stray license blurbs or fragments thereof remain. In two cases
these were extra blurbs in code generators intended for the generated code,
in others they were just missed by the script.
Clean them up, adding an SPDX license identifier where needed.
Closes#10072
This PR rewrites the `utils::result_parallel_for_each`'s implementation to resemble the original `seastar::parallel_for_each` more closely instead of using the less efficient `seastar::map_reduce`. It uses less tasks and allocations now, as demonstrated in the results from the `perf_result_query` benchmark, attached at the end of the cover letter.
The main drawback of the new implementation is that it needs to rethrow exceptions propagated as exceptional futures from the parallel sub-invocations. Contrary to the original `seastar::parallel_for_each` which uses a custom task to collect results, the new `utils::result_parallel_for_each` uses a coroutine and there doesn't currently seem to be a way to co_await for a future and inspect its state without either rethrowing or handling it in then_wrapped (which allocates a continuation). Fortunately, rethrowing is not needed for exceptions returned in failed result<>, which are already intended to be a more performant alternative to regular exceptions.
As a bonus, definitions from `utils/result.hh` are now split across three different headers in order to improve (re)compilation times.
Results from `perf_simple_query --smp 1 --operations-per-shard 1000000 --write` (before vs. after):
```
126872.54 tps ( 67.2 allocs/op, 14.2 tasks/op, 52404 insns/op)
126532.13 tps ( 67.2 allocs/op, 14.2 tasks/op, 52408 insns/op)
126864.99 tps ( 67.2 allocs/op, 14.2 tasks/op, 52428 insns/op)
127073.10 tps ( 67.2 allocs/op, 14.2 tasks/op, 52404 insns/op)
126895.85 tps ( 67.2 allocs/op, 14.2 tasks/op, 52411 insns/op)
127894.02 tps ( 66.2 allocs/op, 13.2 tasks/op, 52036 insns/op)
127671.51 tps ( 66.2 allocs/op, 13.2 tasks/op, 52042 insns/op)
127541.42 tps ( 66.2 allocs/op, 13.2 tasks/op, 52044 insns/op)
127409.10 tps ( 66.2 allocs/op, 13.2 tasks/op, 52052 insns/op)
127831.30 tps ( 66.2 allocs/op, 13.2 tasks/op, 52043 insns/op)
```
Test: unit(dev, debug)
Closes#10053
* github.com:scylladb/scylla:
utils/result: optimize result_parallel_for_each
utils/result: split into `combinators` and `loop` file
This series continues the effort of https://github.com/scylladb/scylla/pull/9844 to reduce `seastar::async` usage and coroutinize in the gossiper code.
There are mostly trivial conversions from using `.get()` to `co_await`, where appropriate, as well, as elimination of `seastar::async()` wrappers.
A few more functions are not yet converted, though (e.g. `apply_new_states`, `do_apply_state_locally`, `apply_state_locally`, `apply_state_locally_without_listener_notification`, maybe a few others, as well).
The motivation is to be able to call every public API function of `gossiper` class without requiring `seastar::async` context.
Tests: unit(debug, dev), dtest (topology-related tests)
Closes#10032
* github.com:scylladb/scylla:
gms: gossiper: coroutinize `wait_for_gossip`
gms: gossiper: coroutinize `advertise_token_removed`
gms: gossiper: coroutinize `advertise_removing`
gms: gossiper: don't wrap `convict` calls into `seastar::async`
gms: gossiper: coroutinize `handle_major_state_change`
gms: gossiper: coroutinize `handle_shutdown_msg`
gms: gossiper: coroutinize `mark_as_shutdown` and `convict`
gms: gossiper: remove comment about requiring thread context in `mark_alive`
gms: gossiper: don't use `seastar::async` in `mark_alive`
gms: gossiper: coroutinize `do_on_change_notifications`
gms: gossiper: coroutinize `do_before_change_notifications`
gms: gossiper: coroutinize `real_mark_alive`
gms: gossiper: coroutinize `mark_dead`
It now resembles the original parallel_for_each more, but uses a
coroutine instead of a custom `task` to collect not-ready futures.
Although the usage of a coroutine saves on allocations, the drawback is
that there is currently no way to co_await on a future and handle its
exception without throwing or without unconditionally allocating a
then_wrapped or handle_exception continuation - so it introduces a
rethrow.
Furthermore, now failed results and exceptions are treated as equals.
Previously, in case one parallel invocation returned failed result and
another returned an exception, the exception would always be returned.
Now, the failed result/exception of the invocation with the lowest index
is always preferred, regardless of the failure type.
The reimplementation manages to save about 350-400 instructions, one
task and one allocation in the perf_simple_query benchmark in write
mode.
Results from `perf_simple_query --smp 1 --operations-per-shard 1000000
--write` (before vs. after):
```
126872.54 tps ( 67.2 allocs/op, 14.2 tasks/op, 52404 insns/op)
126532.13 tps ( 67.2 allocs/op, 14.2 tasks/op, 52408 insns/op)
126864.99 tps ( 67.2 allocs/op, 14.2 tasks/op, 52428 insns/op)
127073.10 tps ( 67.2 allocs/op, 14.2 tasks/op, 52404 insns/op)
126895.85 tps ( 67.2 allocs/op, 14.2 tasks/op, 52411 insns/op)
127894.02 tps ( 66.2 allocs/op, 13.2 tasks/op, 52036 insns/op)
127671.51 tps ( 66.2 allocs/op, 13.2 tasks/op, 52042 insns/op)
127541.42 tps ( 66.2 allocs/op, 13.2 tasks/op, 52044 insns/op)
127409.10 tps ( 66.2 allocs/op, 13.2 tasks/op, 52052 insns/op)
127831.30 tps ( 66.2 allocs/op, 13.2 tasks/op, 52043 insns/op)
```
Test: unit(dev), unit(result_utils_test, debug)
Segregates result utilities into:
- result.hh - basic definitions related to results with exception
containers,
- result_combinators.hh - combinators for working with results in
conjunction with futures,
- result_loop.hh - loop-like combinators, currently has only
result_parallel_for_each.
The motivation for the split is:
1. In headers, usually only result.hh will be needed, so no need to
force most .cc files to compile definitions from other files,
2. Less files need to be recompiled when a combinator is added to
result_combinators or result_loop.
As a bonus, `result_with_exception` was moved from `utils::internal` to
just `utils`.
Adapts the exception handling logic in process_request_one so that it
uses utils::result_try to handle both C++ exceptions and failed results
in a unified way.
Adapts the mutate_end exception handling logic so that it uses the new
utils::result_futurize_try function to handle both exceptional futures
and failed results in an unified way.
Temporarily removes the logic which handles failed results in a
non-throwing way. Exceptions from failed results are thrown and handled
in try..catch.
The reason for this change is that it makes the following commit, which
migrates the whole try..catch block to utils::result_futurize_try much
nicer. The next commit will also bring back the non-throwing handling of
the failed result.
Adds result_try and result_futurize_try - functions which allow to
convert existing try..catch blocks into a version which handles C++
exceptions, failed results with exception containers and, depending on
the function variant, exceptional futures.
Secondary tracing sessions used to compute the execution time
from the point of their `begin()`-ning, not the parent session's
`begin()`. As a result, replica reported a slow query if it
exceeded the entire threshold *on that replica* too.
This change augments `trace_info` with the TS of parent's session
starting point, to be used as a reference on replicas.
Fixes#9403Closes#10005
`system.raft`, `system.raft_snapshots` and `system.raft_config`
were missing from the `extra_durable_tables` list, so that
`set_wait_for_sync_to_commitlog(true)` was not enabled when
the tables were re-created via `create_table_from_mutations`.
Tests: unit(dev)
Signed-off-by: Pavel Solodovnikov <pa.solodovnikov@scylladb.com>
Message-Id: <20220210073418.484843-1-pa.solodovnikov@scylladb.com>
main() has some logic to select the main function it will delegate to
based on argv[1]. The intent is that when the value of argv[1] suggest
that the user did not specify a specific app to run, we default to
"server" (scylla proper).
This logic currently breaks down when there are no arguments at all: in
this case the following error is printed and scylla refuses to start:
error: unrecognized first argument: expected it to be "server", a regular command-line argument or a valid tool name (see `scylla --list-tools`), but got
Fix this by checking for empty argv[1] and defaulting to "server" in
that case.
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20220210092125.293682-1-bdenes@scylladb.com>
This patch adds a "--raft" option to test/alternator/run to enable the
experimental Raft-based schema changes ("--experimental-features=raft")
when running Scylla for the tests. This is the same option we added to
test/cql-pytest/run in a previous patch.
Note that we still don't have any Alternator tests that pass or fail
differently in these two modes - these will probably come later as we
fix issues #9868 and #6391. But in order to work on fixing those issues
we need to be able to run the tests in Raft mode.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220209123144.321344-1-nyh@scylladb.com>
In a previous patch we fixed the output of experimental features list
(issue #10047), so we also need to fix the test code which detects the
"raft" experimental feature - to use the string "raft" and not the
silly byte 4 we had there before.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220209104331.312999-1-nyh@scylladb.com>
