In 2e6755ecca I have added a comment when PR has conflicts so the assignee can get a notification about it. There was a problem with the user mention param (a missing `.login`)
Fixing it
Closesscylladb/scylladb#22036
The main purpose of this change is to enhance the restore from object storage usage.
Currently, restore uses the load-and-stream facility. When triggered, the restoring task opens the provided list of sstables directory from the remote bucket and then feeds the list of sstables to load_and_stream() method. The method, in turn, iterates over this list, reads mutations and for each mutation decides where to send one by checking the replication map (it's pretty much the same for both vnodes and tablets, but for tablets that are "fully contained" by a range there's the plan to stream faster).
As described above, restore is governed by a single node and this single node reads all sstables from the object store, which can be very slow. This PR allows speeding things up. For that, the load-and-stream code is equipped with the "scope" filter which limits where mutations can be streamed to. There are four options for that -- all, dc, rack and node. The "all" is how things work currently, "dc" and "rack" filter out target nodes that don't belong to this node's dc/rack respectively. The "node" scope only streams mutations to local node.
With the "node" scope it's possible to make all nodes in the cluster load mutations that belong to them in parallel, without re-sending them to peers. The last patch in this PR is the test that shows how it can be possible.
Closesscylladb/scylladb#21169
* github.com:scylladb/scylladb:
test: Add scope-streaming test (for restore from backup)
api: New "scope" API param to load-and-stream calls
sstables_loader: Propagate scope from API down
sstables_loader: Filter tablets based on scope
streamer: Disable scoped streaming of primary replica only
sstables_loader: Introduce streaming scope
sstables_loader: Wrap get_endpoints()
It turned out that aforementioned APIs use slightly different sources of information about view build progress/status which sometimes results in different reporting of whether an index is built. It's good to make those two APIs consistent. Also add a test for the REST API endpoint (system table test was addressed by #21677).
Closesscylladb/scylladb#21814
* github.com:scylladb/scylladb:
test: Add tests for MVs and indexes reporting by API endpoint(s)
api: Use built_views table in get_built_indexes API
This series re-implements locator::topology::sort_by_proximity
and adds some randomization to shuffle equal-distance replicas for improving load-balancing
when reading with 1 < consistency level < replication factor.
This change also adds a manual test for benchmarking sort_by_proximity,
as it's not exercised by the single-node perf-simple-query.
The benchmark shows performance improvement of over 20% (from about 71 ns to 56 ns
per call for 3 nodes vectors), mainly due to "calculate distance only once" which
pre-calculates the distance from the reference node for each replica once, rather than
each time to comparator is called by std::sort
* Improvement. No backport needed
Closesscylladb/scylladb#21958
* github.com:scylladb/scylladb:
locator/topology: do_sort_by_proximity: shuffle equal-distance replicas
locator/topology: sort_by_proximity: calculate distance only once
utils: small_vector: expose internal_capacity()
storage_proxy: sort_endpoints_by_proximity: lookup my_id only if cannot sort by proximity
test/perf: add perf_sort_by_proximity benchmark
locator: refactor sort_by_proximity
So far there's the /column_family/built_indexes one that reports the
index names similar to how system.IndexInfo does, but it's not tested.
This patch adds tests next to existing system. table ones.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Somehow system."IndexInfo" table and column_family/built_indexes REST
API endpoint declare an index "built" at slightly different times:
The former a virtual table which declares an index completely built
when it appears on the system.built_views table.
The latter uses different data -- it takes the list of indexes in
the schema and eliminates indexes which are still listed in the
system.scylla_views_builds_in_progress table.
The mentioned system. tables are updated at different times, so API
notices the change a bit later. It's worth improving the consistency
of these two APIs by making the REST API endpoint piggy-back the
load_built_views() instead of load_view_build_progress(). With that
change the filtering of indexes should be negated.
Fixes#21587
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
To improve balancing when reading in 1 < CL < ALL
This implementation has a moderate impact on
the function performance in contrast to full
std::shuffle of the vector before stable_sort:ing it
(especially with large number of nodes to sort).
Before:
test iterations median mad min max allocs tasks inst cycles
sort_by_proximity_topology.perf_sort_by_proximity 25541973 39.225ns 0.114ns 38.966ns 39.339ns 0.000 0.000 588.5 116.6
After:
sort_by_proximity_topology.perf_sort_by_proximity 19689561 50.195ns 0.119ns 50.076ns 51.145ns 0.000 0.000 622.5 150.6
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
And use a temporary vector to use the precalculated distances.
A later patch will add some randomization to shuffle nodes
at the same distance from the reference node.
This improves the function performance by 50% for 3 replicas,
from 77.4 ns to 39.2 ns, larger replica sets show greater improvement
(over 4X for 15 nodes):
Before:
test iterations median mad min max allocs tasks inst cycles
sort_by_proximity_topology.perf_sort_by_proximity 12808773 77.368ns 0.062ns 77.300ns 77.873ns 0.000 0.000 1194.2 231.6
After:
sort_by_proximity_topology.perf_sort_by_proximity 25541973 39.225ns 0.114ns 38.966ns 39.339ns 0.000 0.000 588.5 116.6
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
So we can use it for defining other small_vector
deriving their internal capacity from another small_vector
type.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
topology::sort_by_proximity already sorts the local node
address first, if present, so look it up only when
using SimpleSnitch, where sort_by_proximity() is a no-op.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
benchmark sort_by_proximity
Baseline results on my desktop for sorting 3 nodes:
single run iterations: 0
single run duration: 1.000s
number of runs: 5
number of cores: 1
random seed: 20241224
test iterations median mad min max allocs tasks inst cycles
sort_by_proximity_topology.perf_sort_by_proximity 12808773 77.368ns 0.062ns 77.300ns 77.873ns 0.000 0.000 1194.2 231.6
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
this changeset includes two changes to silence the warnings reported by shellcheck. This changeset has no functional impact and serves as a proactive code improvement.
---
it's a cleanup, hence no need to backport.
Closesscylladb/scylladb#21756
* github.com:scylladb/scylladb:
install-dependencies.sh: quote array to avoid re-splitting
install-dependencies.sh: define local variable using "local -A"
- create
- a cluster with given topology
- keyspace with tablets and given rf value
- table with some data
- backup
- flush all nodes
- kick backup API on every node
- re-create keyspace and table
- drop it first
- create again with the same parameters and schema, but don't
populate table with data
- restore
- collect nodes to contact and corresponding list of TOCs
according to the preferred "scope"
- ask selected nodes to restore, limiting its streaming scope
and providing the specific list of sstables
- check
- select mutation fragments from all nodes for random keys
- make sure that the number of non-empty responses equals the
expected rf value
Specific topologies, RFs and stream scopes used are:
rf = 1, nodes = 3, racks = 1, dcs = 1, scope = node
rf = 3, nodes = 5, racks = 1, dcs = 1, scope = node
rf = 1, nodes = 4, racks = 2, dcs = 1, scope = rack
rf = 3, nodes = 6, racks = 2, dcs = 1, scope = rack
rf = 3, nodes = 6, racks = 3, dcs = 1, scope = rack
rf = 2, nodes = 8, racks = 4, dcs = 2, scope = dc
nodes and racks are evenly distributed in racks and dcs respectively
in the last topo RF effectively becomes 4 (2 in each dc)
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
There are two of those -- the POST /storage_service/keyspace that loads
and streams new sstables from /upload and POST /storage_service/restore
that does the same, but gets sstables from object store.
The new optional parameter allow users to tun the streaming phase
behavior. The test/pylib client part is also updated here.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Semi-mechanical change that adds newly introduced "scope" parameter to
all the functions between API methods and the low-level streamer object.
No real functional changes. API methods set it to "all" to keep existing
behavior.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Loading and streaming tablets has pre-filtering loop that walks the
tablet map sorts sstables into three lists:
- fully contained in one of map ranges
- partially overlapping with the map
- not intersecting with the map
Sstables from the 3rd list is immediately dropped from the process and
for the remaining two core load-and-stream happens.
This filtering deserves more care from the newly introduced scope. When
a tablet replica set doesn't get in the scope, the whole entry can be
disregarded, because load-and-stream will only do its "load" part anyway
and all mutations from it will be ignored.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
There's been some discussions of how primary replica only streaming
schould interact with the scope. There are two options how to consider
this combination:
- find where the primary replica is and handle it if it's within the
requested sope
- within the requested scope find the primary replica for that subset of
nodes, then handle it
There's also some itermediate solution: suppoer "primary replica in DC"
and reject all other combinations.
Until decided which way is correct, let's disable this configuration.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Currently load-and-stream sends mutations to whatever node is considered
to be a "replica" for it. One exception is the "primary-replica-only"
flag that can be requested by the user.
This patch introduces a "scope" parameter that limits streaming part in
where it can stream the data to with 4 options:
- all -- current way of doing things, stream to wherever needed
- dc -- only stream to nodes that live in the same datacenter
- rack -- only stream to nodes that live in the same rack
- node -- only "stream" to current node
It's not yet configurable and streamer object initializes itself with
"all" mode. Will be changed later.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Preparational patch. Next will add more code to get_endpoints() that
will need to work for both if/else branches, this change helps having
less churn later.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Extract can_sort_by_proximity() out so it can be used
later by storage_proxy, and introduce do_sort_by_proximity
that sorts unconditionally.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
To reduce test executable size and speed up compilation time, compile unit
tests into a single executable.
Here is a file size comparison of the unit test executable:
- Before applying the patch
$ du -h --exclude='*.o' --exclude='*.o.d' build/release/test/boost/ build/debug/test/boost/
11G build/release/test/boost/
29G build/debug/test/boost/
- After applying the patch
du -h --exclude='*.o' --exclude='*.o.d' build/release/test/boost/ build/debug/test/boost/
5.5G build/release/test/boost/
19G build/debug/test/boost/
It reduces executable sizes 5.5GB on release, and 10GB on debug.
Closes#9155Closesscylladb/scylladb#21443
Currently, `get_network_topology_options()` is using gossip data
and iterates over topology using IPs and not host IDs, which may
result in operating on inconsistent data.
This method's implemenations has been changed to instead use
`get_datacenters()`, which should always return consistent data.
Fixes: scylladb/scylladb#21490Closesscylladb/scylladb#21940
Where the grammar supports IN, we add NOT IN. This includes the WHERE
clause and LWT IF clause.
Evaluation of NOT IN follows from IN.
In statement_restrictions analysis, they are different, as NOT IN
doesn't enable any clever query plan and must filter.
Some tests are added. An error message was changed ('in' changed to 'IN'),
so some tests are adjusted.
Closesscylladb/scylladb#21992
we already check `self.cmd` for null at the very beginning of the
`ScyllaServer.stop()`, and in the `try` block, we don't reset
`self.cmd`, hence there is no need to check it again.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21936
`prefixed()` is a static function in `mutation_partition_v2.cc`.
and this function is not used in this translation unit. so let's
remove it.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#22006
Currently, in tablet_map_to_mutation, repair's and migration's
tablet_task_info is always set.
Do not set the tablet_task_info if there is no running operation.
Closesscylladb/scylladb#22005
During the consolidation of per-suite pytest.ini files (commit 8bf62a086f),
the 'repair' marker was inadvertently dropped. This led to pytest warnings
for tests using the @pytest.mark.repair decorator.
This patch restores the marker declaration to eliminate the distracting
PytestUnknownMarkWarning:
```
test/topology_experimental_raft/test_tablets.py:396
/home/kefu/dev/scylladb/test/topology_experimental_raft/test_tablets.py:396: PytestUnknownMarkWarning: Unknown pytest.mark.repair - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
@pytest.mark.repair
```
Restoring the marker allows tests to use the 'repair' mark without
generating warnings.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21931
This series converts the call site using compare_endpoints with gms::inet_address.
With that both flavors of compare_endpoints and sort_by_proximity for inet_address
can be retired as no other uses remain.
Also, add a unit test for topology::sort_by_proximity before further changes
to it are considered.
* Code cleanup, no backport is needed
Closesscylladb/scylladb#21976
* github.com:scylladb/scylladb:
test: network_topology_strategy_test: add test_topology_sort_by_proximity
locator/topology: retire sort_by_proximity/compare_endpoints for inet_address
test: test_topology_compare_endpoints: use host_id:s
Today our container is based on ubuntu:22.04, we need to build another container based on Ubuntu Pro for FIPS support (currently the latest one is 20.04)
The default docker build process doesn't change, if FIPS is required I have added `--type pro` to build a supported container.
To enable FIPS there is a need to attach an Ubuntu Pro subscription (it will be done as part of https://github.com/scylladb/scylla-pkg/issues/4186)
Closesscylladb/scylladb#21974
Similar to 9ace191616 (repair: Enable
small table optimization for RBNO bootstrap and decommission), this
patch enables small table optimization for RBNO rebuild.
This is useful for rebuild ops which is used for building an empty DC.
Fixes: #21951Closesscylladb/scylladb#21952
instead of reusing the variable name and overriding the parameter,
use a new name for the return value of `manager_internal()` for better
readability.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21932
these unused includes are identified by clang-include-cleaner. after
auditing the source files, all of the reports have been confirmed.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21837
When an sstable is unlinked, it remains in the _active list of the
sstable manager. Its memory might be reclaimed and later reloaded,
causing issues since the sstable is already unlinked. This patch updates
the on_unlink method to reclaim memory from the sstable upon unlinking,
remove it from memory tracking, and thereby prevent the issues described
above.
Added a testcase to verify the fix.
Fixes#21887
This is a bug fix in the bloom filter reload/reclaim mechanism and should be backported to older versions.
Closesscylladb/scylladb#21895
* github.com:scylladb/scylladb:
sstables_manager: reclaim memory from sstables on unlink
sstables_manager: introduce reclaim_memory_and_stop_tracking_sstable()
sstables: introduce disable_component_memory_reload()
sstables_manager: log sstable name when reclaiming components
In commit bfee93c7, repair verbs were moved to IDL. During this refactoring,
the `gc_clock.hh` header became unused as its references were relocated.
`clang-include-cleaner` helped identify this unnecessary include, which is
now removed to clean up the codebase.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21919
these unused includes are identified by clang-include-cleaner. after
auditing the source files, all of the reports have been confirmed.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21838
now that we are allowed to use C++23. we now have the luxury of using
`std::ranges::stable_partition`.
in this change, we:
- replace `boost::range::stable_parition()` to
`std::ranges::stable_parition()`
- since `std::ranges::stable_parition()` returns a subrange instead of
an iterator, change the names of variables which were previously used
for holding the return value of `boost::range::stable_partition()`
accordingly for better readability.
- remove unused `#include` of boost headers
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21911
Currently, when finishing db::view::calculate_affected_clustering_ranges
we deoverlap, transform and copy all ranges prepared before. This
is all done within a single continuation and can cause stalls.
We fix this by adding yields after each transform and moving elements
to the final vector one by one instead of copying them all at the end.
After this change, the longest continuation in this code will be
deoverlapping the initial ranges (and one transform). While it has
a relatively high computational complexity (we sort all ranges), it
should execute quickly because we're operating on views there and
we don't need to copy the actual bytes. If we encounter a stall there,
we'll need to implement an asynchronous `deoverlap` method.
Fixesscylladb/scylladb#21843Closesscylladb/scylladb#21846
The series contains small fixes to the gossiper one of which fixes#21930. Others I noticed while debugged the issue.
Fixes: scylladb/scylladb#21930Closesscylladb/scylladb#21956
* github.com:scylladb/scylladb:
gossiper: do not reset _just_removed_endpoints in non raft mode
gossiper: do not send echo message to yourself
gossiper: do not call apply for the node's old state
Fixes#20717
Enables abortable interface and propagates abort_source to all s3 objects used for reading the restore data.
Note: because restore is done on each shard, we have to maintain a per-shard abort source proxy for each, and do a background per-shard abort on abort call. This is synced at the end of "run()".
Abort source is added as an optional parameter to s3 storage and the s3 path in distributed loader.
There is no attempt to "clean up" an aborted restore. As we read on a mutation level from remote sstables, we should not cause incomplete sstables as such, even though we might end up of course with partial data restored.
Closesscylladb/scylladb#21567
* github.com:scylladb/scylladb:
test_backup: Add restore abort test case
sstables_loader: Make restore task abortable
distributed_loader: Add optional abort_source to get_sstables_from_object_store
s3_storage: Add optional abort_source to params/object
s3::client: Make "readable_file" abortable
Those are not used anymore now that the last call
site for compare_endpoints by inet_address is converted
to use host_id.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
This is the last call site requiring the compare_endpoints
flavour for inet_address.
Once this test is converted to use host_id:s instead,
compare_endpoints and sort_by_proximity can be simplified
to support only host_id:s.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
When running clang-include-cleaner, the tool performs static analysis by
"compiling" specified source files. Previously, non-existent included headers
caused the tool to skip source files, reducing the effectiveness of unused
include detection.
Problem:
- Header files like 'rust/wasmtime_bindings.hh' were not pre-generated
- Compilation errors led to skipping source file analysis
```
/__w/scylladb/scylladb/lang/wasm.hh:15:10: fatal error: 'rust/wasmtime_bindings.hh' file not found
15 | #include "rust/wasmtime_bindings.hh"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
Skipping file /__w/scylladb/scylladb/lang/wasm.hh due to compiler errors. clang-include-cleaner expects to work on compilable source code.
1 error generated.
```
- This significantly reduced clang-include-cleaner's coverage
Solution:
- Build the `wasmtime_bindings` target to generate required header files
- Ensure all necessary headers are created before running static analysis
- Enable full source file checking for unused includes
By generating headers before analysis, we prevent skipping of source files
and improve the comprehensiveness of our include cleaner workflow.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#21739
This test was added in PR #19789 but was disabled with xfail because of
the bug with way truncate saved the commit log replay positions. More
specifically, the replay positions for shards that had no mutations were
saved to system.truncated with shard_id == 0, regardless for which shard
it was actually saved for (see #21719).
The bug was fixed in #21722, so this change removes the xfail tag from
the test.
Closesscylladb/scylladb#21902
