"
compaction_manager: Avoid stall in perform_cleanup
The following stall was seen during a cleanup operation:
scylla: Reactor stalled for 16262 ms on shard 4.
| std::_MakeUniq<locator::tokens_iterator_impl>::__single_object std::make_unique<locator::tokens_iterator_impl, locator::tokens_iterator_impl&>(locator::tokens_iterator_impl&) at /usr/include/fmt/format.h:1158
| (inlined by) locator::token_metadata::tokens_iterator::tokens_iterator(locator::token_metadata::tokens_iterator const&) at ./locator/token_metadata.cc:1602
| locator::simple_strategy::calculate_natural_endpoints(dht::token const&, locator::token_metadata&) const at simple_strategy.cc:?
| (inlined by) locator::simple_strategy::calculate_natural_endpoints(dht::token const&, locator::token_metadata&) const at ./locator/simple_strategy.cc:56
| locator::abstract_replication_strategy::get_ranges(gms::inet_address, locator::token_metadata&) const at /usr/include/fmt/format.h:1158
| locator::abstract_replication_strategy::get_ranges(gms::inet_address) const at /usr/include/fmt/format.h:1158
| service::storage_service::get_ranges_for_endpoint(seastar::basic_sstring<char, unsigned int, 15u, true> const&, gms::inet_address const&) const at /usr/include/fmt/format.h:1158
| service::storage_service::get_local_ranges(seastar::basic_sstring<char, unsigned int, 15u, true> const&) const at /usr/include/fmt/format.h:1158
| (inlined by) operator() at ./sstables/compaction_manager.cc:691
| (inlined by) _M_invoke at /usr/include/c++/9/bits/std_function.h:286
| std::function<std::vector<seastar::lw_shared_ptr<sstables::sstable>, std::allocator<seastar::lw_shared_ptr<sstables::sstable> > > (table const&)>::operator()(table const&) const at /usr/include/fmt/format.h:1158
| (inlined by) compaction_manager::rewrite_sstables(table*, sstables::compaction_options, std::function<std::vector<seastar::lw_shared_ptr<sstables::sstable>, std::allocator<seastar::lw_shared_ptr<sstables::sstable> > > (table const&)>) at ./sstables/compaction_manager.cc:604
| compaction_manager::perform_cleanup(table*) at /usr/include/fmt/format.h:1158
To fix, we furturize the function to get sstables. If get_local_ranges()
is called inside a thread, get_local_ranges will yield automatically.
Fixes#6662
"
* asias-compaction_fix_stall_in_perform_cleanup:
compaction_manager: Avoid stall in perform_cleanup
compaction_manager: Return exception future in perform_cleanup
abstract_replication_strategy: Add get_ranges_in_thread
"
Currently, the fast forwarding implementation of the shard reader is
broken in some read-ahead related corner cases, namely:
* If the reader was not created yet, but there is an ongoing read-ahead
(which is going to create it), the function bails out. This will
result in this shard reader not being fast-forwarded to the new range
at all.
* If the reader was already created and there is an ongoing read-ahead,
the function will wait for this to complete, then fast-forward the
reader, as it should. However, the buffer is cleared *before* the
read-ahead is waited for. So if the read-ahead brings in new data,
this will land in the buffer. This data will be outside of the
fast-forwarded-to range and worse, as we just cleared the buffer, it
might violate mutation fragment stream monotonicity requirements.
This series fixes these two bugs and adds a unit test which reproduces
both of them.
There are no known field issues related to these bugs. Only row-level
repair ever fast-forwards the multishard reader, but it only uses it in
heterogenous clusters. Even so, in theory none of these bugs affect
repair as it doesn't ever fast-forward the multishard reader before all
shards arrive at EOS.
The bugs were found while auditing the code, looking for the possible
cause of #6613.
Fixes: #6715
Tests: unit(dev)
"
* 'multishard-combining-reader-fast-forward-to-fixes/v1.1' of https://github.com/denesb/scylla:
test/boost/mutation_reader_test: multishard reader: add tests for fast-forwarding with read-ahead
test/boost/mutation_reader_test: extract multishard read-ahead test setup
test/boost/mutation_reader_test: puppet_reader: fast-forward-support
mutation_reader_test: puppet_reader: make interface more predictable
dht::sharder: add virtual destructor
mutation_reader: shard_reader: fix fast-forwarding with read-ahead
Testing the multishard reader's various read-ahead related corner cases
requires a non-trivial setup. Currently there is just one such test,
but we plan to add more so in this patch we extract this setup code to a
free function to allow reuse across multiple tests.
A fast-forwarded puppet reader goes immediately to EOS. A counter is
added to the remote control to allow tests to check which readers were
actually fast forwarded.
Currently the puppet reader will do an automatic (half) buffer-fill in
the constructor. This makes it very hard to reason about when and how
the action that was passed to it will be executed. Refactor it to take a
list of actions and only execute those, no hidden buffer-fill anymore.
No better proof is needed for this than the fact that the test which is
supposed to test the multishard reader being destroyed with a pending
read-ahead was silently broken (not testing what it should).
This patch fixes this test too.
Also fixed in this patch is the `pending` and `destroyed` fields of the
remote control, tests can now rely on these to be correct and add
additional checkpoints to ensure the test is indeed doing what it was
intended to do.
The following stall was seen during a cleanup operation:
scylla: Reactor stalled for 16262 ms on shard 4.
| std::_MakeUniq<locator::tokens_iterator_impl>::__single_object std::make_unique<locator::tokens_iterator_impl, locator::tokens_iterator_impl&>(locator::tokens_iterator_impl&) at /usr/include/fmt/format.h:1158
| (inlined by) locator::token_metadata::tokens_iterator::tokens_iterator(locator::token_metadata::tokens_iterator const&) at ./locator/token_metadata.cc:1602
| locator::simple_strategy::calculate_natural_endpoints(dht::token const&, locator::token_metadata&) const at simple_strategy.cc:?
| (inlined by) locator::simple_strategy::calculate_natural_endpoints(dht::token const&, locator::token_metadata&) const at ./locator/simple_strategy.cc:56
| locator::abstract_replication_strategy::get_ranges(gms::inet_address, locator::token_metadata&) const at /usr/include/fmt/format.h:1158
| locator::abstract_replication_strategy::get_ranges(gms::inet_address) const at /usr/include/fmt/format.h:1158
| service::storage_service::get_ranges_for_endpoint(seastar::basic_sstring<char, unsigned int, 15u, true> const&, gms::inet_address const&) const at /usr/include/fmt/format.h:1158
| service::storage_service::get_local_ranges(seastar::basic_sstring<char, unsigned int, 15u, true> const&) const at /usr/include/fmt/format.h:1158
| (inlined by) operator() at ./sstables/compaction_manager.cc:691
| (inlined by) _M_invoke at /usr/include/c++/9/bits/std_function.h:286
| std::function<std::vector<seastar::lw_shared_ptr<sstables::sstable>, std::allocator<seastar::lw_shared_ptr<sstables::sstable> > > (table const&)>::operator()(table const&) const at /usr/include/fmt/format.h:1158
| (inlined by) compaction_manager::rewrite_sstables(table*, sstables::compaction_options, std::function<std::vector<seastar::lw_shared_ptr<sstables::sstable>, std::allocator<seastar::lw_shared_ptr<sstables::sstable> > > (table const&)>) at ./sstables/compaction_manager.cc:604
| compaction_manager::perform_cleanup(table*) at /usr/include/fmt/format.h:1158
To fix, we furturize the function to get local ranges and sstables.
In addition, this patch removes the dependency to global storage_service object.
Fixes#6662
The current `fast_forward_to(const dht::partition_range&)`
implementation has two problems:
* If the reader was not created yet, but there is an ongoing read-ahead
(which is going to create it), the function bails out. This will
result in this shard reader not being fast-forwarded to the new range
at all.
* If the reader was already created and there is an ongoing read-ahead,
the function will wait for this to complete, then fast-forward the
reader, as it should. However, the buffer is cleared *before* the
read-ahead is waited for. So if the read-ahead brings in new data,
this will land in the buffer. This data will be outside of the
fast-forwarded-to range and worse, as we just cleared the buffer, it
might violate mutation fragment stream monotonicity requirements.
This patch fixes both of these bugs. Targeted reproducer unit tests are
coming in the next patches.
When we started to porting bash script to python script, we are not able to use
subprocess.run() since EPEL only provides python 3.4, but now we have
relocatable python, so we can switch to it.
* seastar 5db34ea8d3...dbecfff5a4 (3):
> sharded: Do not hang on never set freed promise
Fixes#6606.
> foreign_ptr: make constructors and methods conditionally noexcept
> foreign_ptr: specify methods as noexcept
Currently the section on "Debugging coredumps" only briefly mentions
relocatable binaries, then starts with an extensive subsection on how to
open cores generated by non-relocatable binaries. There is a subsection
about relocatable binaries, but it just contains some out-of-date
workaround without any context.
In this patch we completely replace this outdated and not very useful
subsection on relocatable binaries, with a much more extensive one,
documenting step-by-step a procedure that is known to work. Also, this
subsection is moved above the non-relocatable one. All our current
releases except for 2019.1 use relocatable binaries, so the subsection
about these should be the more prominent one.
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20200630145655.159926-1-bdenes@scylladb.com>
* seastar 11e86172ba...5db34ea8d3 (7):
> scollectd: Avoid a deprecated warning
> prometheus: Avoid protobuf deprecated warning
> Merge "Avoid abandoned futures in tests" from Rafael
> Merge "make circular_buffer methods noexcept" from Benny
> futures_test: Wait for future in test
> iotune: Report disk IOPS instead of kernel IOPS
> io_tester: Ability to add dsync option to open_file_dma
needs_cleanup() returns true if a sstable needs cleanup.
Turns out it's very slow because it iterates through all the local
ranges for all sstables in the set, making its complexity:
O(num_sstables * local_ranges)
We can optimize it by taking into account that abstract_replication_strategy
documents that get_ranges() will return a list of ranges that is sorted
and non-overlapping. Compaction for cleanup already takes advantage of that
when checking if a given partition can be actually purged.
So needs_cleanup() can be optimized into O(num_sstables * log(local_ranges)).
With num_sstables=1000, RF=3, then local_ranges=256(num_tokens)*3, it means
the max # of checks performed will go from 768000 to ~9584.
Fixes#6730.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20200629171355.45118-2-raphaelsc@scylladb.com>
"
Recent branch introduced uncaught by regular snapshot tests issue
with snapshots listing via API, this set fixes it (patch #1),
cleans the indentation after the fix (#2) and polishes the way
stuff is captured nearby (#3)
Tests: dtest(nodetool_additional_tests)
"
* 'br-fix-snap-get-details' of https://github.com/xemul/scylla:
api: Remove excessive capture
api: Fix indentation after previous patch
api: Fix wrongly captured map of snapshots
Currently fmt::print is used to print an error message
if (broadcast_address != listen && seeds.count(listen))
and the logger should be used instead.
While at it, the information printed in this message is valueable
also in the error-free case, so this change logs it at `info`
level and then logs an error without repeating the said info.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Test: bootstrap_test.py:TestBootstrap.start_stop_test_node(dev)
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20200630083826.153326-1-bhalevy@scylladb.com>
"
These patches removes the last few uses of variadic futures in scylla.
"
* 'espindola/no-variadic-future' of https://github.com/espindola/scylla:
row level repair: Don't return a variadic future from get_sink_source
row level repair: Don't return a variadic future from read_rows_from_disk
messaging_service: Don't return variadic futures from make_sink_and_source_for_*
cql3: Don't use variadic futures in select_statement
"
Offstrategy work, on boot and refresh, guarantees that a shared SSTable
will not reach the table whatsoever. We have lots of extra code in
table to make it able to live with those shared SSTables.
Now we can fortunately get rid of all that code.
tests: mode(dev).
also manually tested it by triggering resharding both on boot/refresh.
"
* 'cleanup_post_offstrategy_v2' of https://github.com/raphaelsc/scylla:
distributed_loader: kill unused invoke_shards_with_ptr()
sstables:: kill unused sstables::sstable_open_info
sstables: kill unused sstable::load_shared_components()
distributed_loader: remove declaration of inexistent do_populate_column_family()
table: simplify table::discard_sstables()
table: simplify add_sstable()
table: simplify update_stats_for_new_sstable()
table: remove unused open_sstable function
distributed_loader: remove unused code
table: no longer keep track of sstables that need resharding
table: Remove unused functions no longer used by resharding
table: remove sstable::shared() condition from backlog tracker add/remove functions
table: No longer accept a shared SSTable
This patch set adds a few new features in order to fix issue
The list of changes is briefly as follows:
- Add a new `LWT` flag to `cql3::prepared_metadata`,
which allows clients to clearly distinguish betwen lwt and
non-lwt statements without need to execute some custom parsing
logic (e.g. parsing the prepared query with regular expressions),
which is obviously quite fragile.
- Introduce the negotiation procedure for cql protocol extensions.
This is done via `cql_protocol_extension` enum and is expected
to have an appropriate mirroring implementation on the client
driver side in order to work properly.
- Implmenent a `LWT_ADD_METADATA_MARK` cql feature on top of the
aforementioned algorithm to make the feature negotiable and use
it conditionally (iff both server and client agrees with each
other on the set of cql extensions).
The feature is meant to be further utilized by client drivers
to use primary replicas consistently when dealing with conditional
statements.
* git@github.com:ManManson/scylla feature/lwt_prepared_meta_flag_2:
lwt: introduce "LWT" flag in prepared statement metadata
transport: introduce `cql_protocol_extension` enum and cql protocol extensions negotiation
Currently we we mistakenly made two different way to detect distribution,
directly reading /etc/os-release and use distro package.
distro package provides well abstracted APIs and still have full access to
os-release informations, we should switch to it.
Fixes#6691
After commit 7d86a3b208 (storage_service:
Make replacing node take writes), during replace operation, tokens in
_token_metadata for node being replaced are updated only after the replace
operation is finished. As a result, in range_streamer::add_ranges, the
node being replaced will be considered as a source to stream data from.
Before commit 7d86a3b208, the node being
replaced will not be considered as a source node because it is already
replaced by the replacing node before the replace operation is finished.
This is the reason why it works in the past.
To fix, filter out the node being replaced as a source node explicitly.
Tests: replace_first_boot_test and replace_stopped_node_test
Backports: 4.1
Fixes: #6728
get_shards_for_this_sstable() can be called inside table::add_sstable()
because the shards for a sstable is precomputed and so completely
exception safe. We want a central point for checking that table will
no longer added shared SSTables to its sstable set.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
no longer need to conditionally track the SSTable metadata,
as table will no longer accept shared SSTables.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Now that table will no longer accept shared SSTables, it no longer
needs to keep track of them.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Now that table no longer accept shared SSTables, those two functions can
be simplified by removing the shared condition.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
With off-strategy work on reshard on boot and refresh, table no
longer needs to work with Shared SSTables. That will unlock
a host of cleanups.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
The results of get_snapshot_details() is saved in do_with, then is
captured on the json callback by reference, then the do_with's
future returns, so by the time callback is called the map is already
free and empty.
Fix by capturing the result directly on the callback.
Fixes recently merged b6086526.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This patch adds a new `LWT` flag to `cql3::prepared_metadata`.
That allows clients to clearly distinguish betwen lwt and
non-lwt statements without need to execute some custom parsing
logic (e.g. parsing the prepared query with regular expressions),
which is obviously quite fragile.
The feature is meant to be further utilized by client drivers
to use primary replicas consistently when dealing with conditional
statements.
Whether to use lwt optimization flag or not is handled by negotiation
procedure between scylla server and client library via SUPPORTED/STARTUP
messages (`LWT_ADD_METADATA_MARK` extension).
Tests: unit(dev, debug), manual testing with modified scylla/gocql driver
Signed-off-by: Pavel Solodovnikov <pa.solodovnikov@scylladb.com>
