Just in case the new algorithm turns out to be buggy, or give a
performance regression, add a flag to fall-back to the old algorithm for
use in the field.
Closes#9908
* github.com:scylladb/scylla:
db: config: add a flag to disable new reversed reads algorithm
replica: table: remove obsolete comment about reversed reads
This series greatly reduces gossipers' dependence on `seastar::async` (yet, not completely).
`i_endpoint_state_change_subscriber` callbacks are converted to return futures (again, to get rid of `seastar::async` dependency), all users are adjusted appropriately (e.g. `storage_service`, `cdc::generation_service`, `streaming::stream_manager`, `view_update_backlog_broker` and `migration_manager`).
This includes futurizing and coroutinizing the whole function call chain up to the `i_endpoint_state_change_subscriber` callback functions.
To aid the conversion process, a non-`seastar::async` dependent variant of `utils::atomic_vector::for_each` is introduced (`for_each_futurized`). A different name is used to clearly distinguish converted and non-converted code, so that the last step (remove `seastar::async()` wrappers around callback-calling code in gossiper) is easier. This is left for a follow-up series, though.
Tests: unit(dev)
Closes#9844
* github.com:scylladb/scylla:
service: storage_service: coroutinize `set_gossip_tokens`
service: storage_service: coroutinize `leave_ring`
service: storage_service: coroutinize `handle_state_left`
service: storage_service: coroutinize `handle_state_leaving`
service: storage_service: coroutinize `handle_state_removing`
service: storage_service: coroutinize `do_drain`
service: storage_service: coroutinize `shutdown_protocol_servers`
service: storage_service: coroutinize `excise`
service: storage_service: coroutinize `remove_endpoint`
service: storage_service: coroutinize `handle_state_replacing`
service: storage_service: coroutinize `handle_state_normal`
service: storage_service: coroutinize `update_peer_info`
service: storage_service: coroutinize `do_update_system_peers_table`
service: storage_service: coroutinize `update_table`
service: storage_service: coroutinize `handle_state_bootstrap`
service: storage_service: futurize `notify_*` functions
service: storage_service: coroutinize `handle_state_replacing_update_pending_ranges`
repair: row_level_repair_gossip_helper: coroutinize `remove_row_level_repair`
locator: reconnectable_snitch_helper: coroutinize `reconnect`
gms: i_endpoint_state_change_subscriber: make callbacks to return futures
utils: atomic_vector: introduce future-returning `for_each` function
utils: atomic_vector: rename `for_each` to `thread_for_each`
gms: gossiper: coroutinize `start_gossiping`
gms: gossiper: coroutinize `force_remove_endpoint`
gms: gossiper: coroutinize `do_status_check`
gms: gossiper: coroutinize `remove_endpoint`
replica::database is (as its name indicates) a replica-side service, while thrift
is coordinator-side. Convert thrift's use of replica::database for data dictionary
lookups to the data_dictionary module. Since data_dictionary was missing a
get_keyspaces() operation, add that.
Thrift still uses replica::database to get the schema version. That should be
provided by migration_manager, but changing that is left for later.
Closes#9888
* github.com:scylladb/scylla:
thrift: switch from replica module to data_dictionary module
thrift: simplify execute_schema_command() calling convention
data_dictionary: add get_keyspaces() method
The BatchGetItem request can return a very large response - according to
DynamoDB documentation up to 16 MB, but presently in Alternator, we allow
even more (see #5944).
The problem is that the existing code prepares the entire response as
a large contiguous string, resulting in oversized allocation warnings -
and potentially allocation failures. So in this patch we estimate the size
of the BatchGetItem response, and if it is "big enough" (currently over
100 KB), we return it with the recently added streaming output support.
This streaming output doesn't avoid the extra memory copies unfortunately,
but it does avoid a *contiguous* allocation which is the goal of this
patch.
After this patch, one oversized allocation warning is gone from the test:
test/alternator/run test_batch.py::test_batch_get_item_large
(a second oversized allocation is still present, but comes from the
unrelated BatchWriteItem issue #8183).
Fixes#8522
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20220111170541.637176-1-nyh@scylladb.com>
Just in case the new algorithm turns out to be buggy, or give a
performance regression, add a flag to fall-back to the old algorithm for
use in the field.
Thrift is a coordinator-side service and should not touch the replica
module. Switch it to data_dictionary.
The switch is straightforward with two exceptions:
- client_state still receives replica::database parameters. After
this change it will be easier to adapt client_state too.
- calls to replica::database::get_version() remain. They should be
rerouted to migration_manager instead, as that deals with schema
management.
execute_schema_command is always called with the same first two
parameters, which are always defined froom the thrift_handler
instance that contains its caller. Simplify it by making it a member
function.
This simplifies migration to data_dictionary in the next patch.
Mirroring replica::database::get_keyspaces(), for Thrift's use.
We return a vector instead of a hash map. Random access is already
available via database::find_keyspace(). The name is available
via the keyspace metadata, and in fact Thrift ignore the map
name and uses the metadata name. Using a simpler type reduces
include dependencies for this heavily used module.
The function is plumbed to replica::database::get_keyspaces() so
it returns the same data.
The patch series moves the rest of internal ddl users to do schema
change over raft (if enabled). After that series only tests are left
using old API.
* 'gleb/raft-schema-rest-v6' of github.com:scylladb/scylla-dev: (33 commits)
migration_manager: drop no longer used functions
system_distributed_keyspace: move schema creation code to use raft
auth: move table creation code to use raft
auth: move keyspace creation code to use raft
table_helper: move schema creation code to use raft
cql3: make query_processor inherit from peering_sharded_service
table_helper: make setup_table() static
table_helper: co-routinize setup_keyspace()
redis: move schema creation code to go through raft
thrift: move system_update_column_family() to raft
thrift: authenticate a statement before verifying in system_update_column_family()
thrift: co-routinize system_update_column_family()
thrift: move system_update_keyspace() to raft
thrift: authenticate a statement before verifying in system_update_keyspace()
thrift: co-routinize system_update_keyspace()
thrift: move system_drop_keyspace() to raft
thrift: authenticate a statement before verifying in system_drop_keyspace()
thrift: co-routinize system_drop_keyspace()
thrift: move system_add_keyspace() to raft
thrift: co-routinize system_add_keyspace()
...
This was needed to fix issue #2129 which was only manifest itself with
auto_bootstrap set to false. The option is ignored now and we always
wait for schema to synch during boot.
Fixes#9408
While it is rare, some customer issues have shown that we can run into cases where commit log apply (writing mutations to it) fails badly. In the known cases, due to oversized mutations. While these should have been caught earlier in the call chain really, it would probably help both end users and us (trying to figure out how they got so big and how they got so far) iff we added info to the errors thrown (and printed), such as ks, cf, and mutation content.
Somewhat controversial, this makes the apply with CL decision path coroutinized, mainly to be able to do the error handling for the more informative wrapper exception easier/less ugly. Could perhaps do with futurize_invoke + then_wrapper also. But future is coroutines...
This is as stated somewhat problematic, it adds an allocation to perf_simple_query::write path (because of crap clang cr frame folding?). However, tasks/op remain constant and actual tps (though unstable) remain more or less the same (on my crappy measurements).
Counter path is unaffected, as coroutine frame alloc replaces with(...)
dtest for the wrapped exception on separate pr.
Closes#9412
* github.com:scylladb/scylla:
database: Add error message with mutation info on commit log apply failure
database: coroutinize do_apply and apply_with_commitlog
Fixes#9408
While it is rare, some customer issues have shown that we can run into cases
where commit log apply (writing mutations to it) fails badly. In the known
cases, due to oversized mutations. While these should have been caught earlier
in the call chain really, it would probably help both end users and us (trying
to figure out how they got so big and how they got so far) iff we added info
to the errors thrown (and printed), such as ks, cf, and mutation content.
Somewhat controversial. Making the apply with CL decision path
coroutinized, mainly to be able to in next patch make error handling
more informative (because we will have exceptions that are immediate
and/or futurized).
This is as stated somewhat problematic, it adds an allocation to
perf_simple_query::write path (because of crap clang cr frame folding?).
However, tasks/op remain constant and actual tps (though unstable)
remain more or less the same (on my crappy measurements).
Counter path is unaffected, as coroutine frame alloc replaces with(...)
alloc, and all is same and dandy.
I am hoping that the simpler error + verbose code will compensate for
the extra alloc.
Refs: #9555
When running the "Kraken" dynamodb streams test to provoke the issued observed by QA, I noticed on my setup mainly two things: Large allocation stalls (+ warnings) and timeouts on read semaphores in DB.
This tries to address the first issue, partly by making query_result_view serialization using chunked vector instead of linear one, and by introducing a streaming option for json return objects, avoiding linearizing to string before wire.
Note that the latter has some overhead issues of its own, mainly data copying, since we essentially will be triple buffering (local, wrapped http stream, and final output stream). Still, normal string output will typically do a lot of realloc which is potential extra copies as well, so...
This is not really performance tested, but with these tweaks I no longer get large alloc stalls at least, so that is a plus. :-)
Closes#9713
* github.com:scylladb/scylla:
alternator::executor: Use streamed result for scan etc if large result
alternator::streams: Use streamed result in get_records if large result
executor/server: Add routine to make stream object return
rjson: Add print to stream of rjson::value
query_idl: Make qr_partition::rows/query_result::partitions chunked
