Currently start/stop of cql is protected with storage_service's
run_with_api_lock, but this protection is purely needed to
guard start and stop against each other, not from anything else.
For the sake of cql server isolation it's worth having its own
start-stop lock. This also decouples cql code from storage_service's
"isolated" thing.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The plan is to decouple cql server management code from
storage_service and move into transport/ directory, so
prepare for that by introducing a controller class.
This leaves some unclean indentation in start/stop helpers
to reduce the churn, it will be fixed two patches ahead.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Currntly API endpoints to start and stop cql_server and thrift
are registered right after the storage service is started, but
much earlier than those services are. In between these two
points a lot of other stuff gets initialized. This opens a small
window during which cql_server and thrift can be started by
hand too early.
The most obvious problem is -- the storage_service::join_cluster()
may not yet be called, the auth service is thus not started, but
starting cql/thrift needs auth.
Another problem is those endpoints are not unregistered on stop,
thus creating another way to start cql/thrif at wrong time.
Also the endpoints registration change helps further patching.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
tracked_file_impl is a wrapper around another file, that tracks
memory allocated for buffers in order to control memory consumption.
However, it neglects to inherit the disk and memory alignment settings
from the wrapped file, which can cause unnecessarily-large buffers
to be read from disk, reducing throughput.
Fix by copying the alignment parameters.
Fixes#6290.
To reduce special cases for the build bots, default dpdk to enabled
in release mode, keeping it disabled for debug and dev.
To allow release modes without dpdk to be build, the --enable-dpdk
switch is converted to a tri-state. When disabled, dpdk is disabled
across all modes. Similarly when enabled the effect is global. When
unspecified, dpdk is enabled for release mode only.
After this change, reloc/build_reloc.sh no longer needs to specify
--enable-dpdk, so remove it.
The messaging service constructor's body does two main things in this
order:
1. it registers the CLIENT_ID verb with rpc.
2. it initializes the scheduling mechanism in charge of locating the
right scheduling group for each verb.
The registration function uses the scheduling mechanism to determine
the scheduling group for the verb.
This commit simply reverses the order of execution.
Fixes#6628
When compaction A completes, a request is issued so that all parallel compactions
will replace compaction A's input sstables by respective output sstables, in the
SSTable set snapshot used for expiration purposes.
That's done to allow space of input SSTables to be released as soon as possible,
helping a lot incremental compaction, but also the non-incremental approach.
Recently I came to realization that we're copying the SSTable set, when doing the
replacement, to make the code exception safe, but it turns out that if an exception
is triggered, the compaction will fail anyway. So this copy is very useless and a
potential source of reactor stalls if strategies like LCS is used.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20200608192614.9354-1-raphaelsc@scylladb.com>
"
After "Make replacing node take writes" series, with repair based node
operations disabled, we saw the replace operation fail like:
```
[shard 0] init - Startup failed: std::runtime_error (unable to find
sufficient sources for streaming range (9203926935651910749, +inf) in
keyspace system_auth)
```
The reason is the system_auth keyspace has default RF of 1. It is
impossible to find a source node to stream from for the ranges owned by
the replaced node.
In the past, the replace operation with keyspace of RF 1 passes, because
the replacing node calls token_metadata.update_normal_tokens(tokens,
ip_of_replacing_node) before streaming. We saw:
```
[shard 0] range_streamer - Bootstrap : keyspace system_auth range
(-9021954492552185543, -9016289150131785593] exists on {127.0.0.6}
```
Node 127.0.0.6 is the replacing node 127.0.0.5. The source node check in
range_streamer::get_range_fetch_map will pass if the source is the node
itself. However, it will not stream from the node itself. As a result,
the system_auth keyspace will not get any data.
After the "Make replacing node take writes" series, the replacing node
calls token_metadata.update_normal_tokens(tokens, ip_of_replacing_node)
after the streaming finishes. We saw:
```
[shard 0] range_streamer - Bootstrap : keyspace system_auth range
(-9049647518073030406, -9048297455405660225] exists on {127.0.0.5}
```
Since 127.0.0.5 was dead, the source node check failed, so the bootstrap
operation.
Ta fix, we ignore the table of RF 1 when it is unable to find a source
node to stream.
Fixes#6351
"
* asias-fix_bootstrap_with_rf_one_in_range_streamer:
range_streamer: Handle table of RF 1 in get_range_fetch_map
streaming: Use separate streaming reason for replace operation
We were not consistent about using '#include "foo.hh"' instead of
'#include <foo.hh>' for scylla's own headers. This patch fixes that
inconsistency and, to enforce it, changes the build to use -iquote
instead of -I to find those headers.
Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
Message-Id: <20200608214208.110216-1-espindola@scylladb.com>
There is no point to hold prepared_metadata in result_message::prepared
as a shared_ptr since their lifetime match.
Message-Id: <20200610113217.GF335449@scylladb.com>
The test test_key_condition_expression_multi() had a small typo, which
was hidden by the fact that the request was expected to fail for other
reasons, but nevertheless should be fixed.
Moreover, it appears that the Amazon DynamoDB changed their error message
for this case, so running the test with "--aws" failed. So this patch
makes it work again by being more forgiving on the exact error message.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200609205628.562351-1-nyh@scylladb.com>
In the existing Alternator code, we used std::unique_ptr<rjson::value> for
passing the optional old value of an item read for a RMW operation.
The benfit of this type over the simpler "const rjson::value*" is that it
gives the callee ownership of the item, and thus the ability to move parts
of it into the response without copying them. We only used this ability in a
handful of obscure cases involving ReturnedValues, but I am not going to
break this dubious feature in this patch.
Nevertheless, a lot of internal code, like condition checks, just needs
read-only access to that previous item, so we passed a reference to the
unique_ptr, i.e., "const std::unique_ptr<rjson::value>&". This is ugly,
and also forces new code that wants to use the same condition checks (i.e.,
filtering code), to artificially allocate a unique_ptr just because that
is what these functions expect.
So in this patch, we change the utility functions such as
verify_condition_expression() and everything they use, to pass around a
"const rjson::value*" instead of a "const std::unique_ptr<rjson::value>&.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200604131352.436506-1-nyh@scylladb.com>
Since we don't support Ubuntu 14.04 anymore, we can drop Upstart related code
from supervisor.[cc|hh].
Also, "#ifdef HAVE_LIBSYSTEMD" was for compiling Scylla on older distribution
which does not provide libsystemd, we nolonger need this since we always build
Scylla on latest Fedora.
Dropping HAVE_LIBSYSTEMD also means removing libsystemd from optional_packages
in configure.py, make it required library.
Note that we still may run Scylla without systemd such as our Docker image,
but sd_notify() does nothing when systemd does not detected, so we can ignore
such case.
Reference: https://www.freedesktop.org/software/systemd/man/sd_notify.html
Reference: https://github.com/systemd/systemd/blob/master/src/libsystemd/sd-daemon/sd-daemon.c
Amazon Linux 2 has /usr/bin/cpupower, but does not have cpupower.service
unlike CentOS7.
We need to provide the .service file when distribution is Amazon Linux 2.
Fixes#5977
Current sender sends stream_mutation_fragments_cmd::end_of_stream to
receiver when an error is received from a peer node. To be safe, send
stream_mutation_fragments_cmd::error instead of
stream_mutation_fragments_cmd::end_of_stream to prevent end_of_stream to
be written into the sstable when a partition is not closed yet.
In addition, use mutation_fragment_stream_validator to valid the
mutation fragments emitted from the reader, e.g., check if
partition_start and partition_end are paired when the reader is done. If
not, fail the stream session and send
stream_mutation_fragments_cmd::error instead of
stream_mutation_fragments_cmd::end_of_stream to isolate the problematic
sstables on the sender node.
Refs: #6478
"
This series allows for resharding SSTables (if needed) before SSTables are
moved from the upload directory, instead of after.
The infrastructure is supposed to be used soon to also load SSTables at boot.
That, however, will take a bit longer as we need to reshape resharded SSTables
for maximum benefit. That should benefit the upload directory as well, however
the current series already presents high incremental value for upload directory
and could be merged sooner (so I can focus on reshaping).
For now, this series still keep the actual moving from upload directory
to the main directory untouched. Once reshaping is ready, it will take
care of this too.
A new file with tests is introduced that tests the process of reading
SSTables from an existing directory.
dtests executed: migration_test.py (--smp 4), which previously failed
"
* 'upload-reshard-v8.1' of github.com:glommer/scylla:
load_new_sstables: reshard before scanning the upload directory
distributed_load: initial handling of off-strategy SSTables
remove manifest_file filter from table.
sstables: move open-related structures to their own file.
sstables: store data size in foreign_sstable_open_info
compaction: split compaction.hh header
In a later patch we will be able move files directly from upload
into the main directory. However for now, for the benefit of doing
this incrementally, we will first reshard in place with our new
reshard infrastructure.
load_new_sstables can then move the SSTables directly, without having
to worry about resharding. This has the immediate benefit that the
resharding happens:
- in the streaming group, without affecting compaction work
- without waiting for the current locks (which are held by compactions)
in load_new_sstables to release.
We could, at this point, just move the SSTables to the main directory
right away.
I am not doing this in this patch, and opting to keep the rest of upload
process unchanged. This will be fixed later when we enable offstrategy
compactions: we'll then compact the SSTables generated into the main
directory.
Signed-off-by: Glauber Costa <glauber@scylladb.com>
Fixes#6561
Pre-image generation in row deletion case only checked if we had a pre-image
result set row. But that can be from post-image. Also check actual existance
of the pre-image CK.
Message-Id: <20200608132804.23541-1-calle@scylladb.com>
Off-strategy SSTables are SSTables that do not conform to the invariants
that the compaction strategies define. Examples of offstrategy SSTables
are SSTables acquired over bootstrap, resharding when the cpu count
changes or imported from other databases through our upload directory.
This patch introduces a new class, sstable_directory, that will
handle SSTables that are present in a directory that is not one of the
directories where the table expects its SSTables.
There is much to be done to support off-strategy compactions fully. To
make sure we make incremental progress, this patch implements enough
code to handle resharding of SSTables in the upload directory. SSTables
are resharded in place, before we start accessing the files.
Later, we will take other steps before we finally move the SSTables into
the main directory. But for now, starting with resharding will not only
allow us to start small, but it will also allow us to start unleashing
much needed cleanups in many places. For instance, once we start
resharding on boot before making the SSTables available, we will be able
to expurge all places in Scylla where, during normal operations, we have
extra handler code for the fact that SSTables could be shared.
Tests: a new test is added and it passes in debug mode.
Signed-off-by: Glauber Costa <glauber@scylladb.com>
When we are scanning an sstable directory, we want to filter out the
manifest file in most situations. The table class has a filter for that,
but it is a static filter that doesn't depend on table for anything. We
are better off removing it and putting in another independent location.
While it seems wasteful to use a new header just for that, this header
will soon be populated with the sstable_directory class.
Tests: unit (dev)
Signed-off-by: Glauber Costa <glauber@scylladb.com>
sstables/sstables.hh is one of our heaviest headers and it's better that we don't
include it if possible. For some users, like distributed_loader, we are mostly
interested in knowing the shape of structures used to open an SSTable.
They are:
- the entry_descriptor, representing an SSTable that we are scanning on-disk
- the sstable_open_info, representing information about a local, opened SSTable
- the foreign_sstable_open_info, representing information about an opened SSTable
that can cross shard boundaries.
Signed-off-by: Glauber Costa <glauber@scylladb.com>
In the new version of resharding we'll want to spread SSTables around
the many shards based on their total size. This means we also need to
know the size of each SSTable individually.
We could wrap the foreign_sstable_info around another structure that
keeps track of that, but because this structure exists mostly for
resharding purposes anyway we will just add the data_size to it.
Signed-off-by: Glauber Costa <glauber@scylladb.com>
compaction.hh is one of our heavy headers, but some users just want to
use information on it about how to describe a compaction, not how to
perform one.
For that reason this patch splits the compaction_descriptor into a new
header.
The compaction_descriptor has, as a member type, compaction_options.
That is moved too, and brings with it the compaction_type. Both of those
structures would make sense in a separate header anyway.
The compaction_descriptor also wants the creator_fn and replacer_fn
functions. We also take this opportunity to rename them into something
more descriptive
Signed-off-by: Glauber Costa <glauber@scylladb.com>
same as redhat, makeself script changes current umask, scylla_setup causes
"scylla does not work with current umask setting (0077)" error.
To fix that we need use latest version of makeself, and specfiy --keep-umask
option.
See #6243
Unlike redhat version, debian version already supported cross build since
it uses debootstrap, but the shellscript rejecting to continue build on
non-debian distribution, so drop these lines to build on Fedora.
[avi: regenerate toolchain]
This is scylla-python3 version of #6611, but we also need to rename
.deb build directory for scylla-python3, since we may lose .deb when
building both scylla and scylla-python3 .deb package, since we currently
sharing build directory.
So renamed it to build/python3/debian.
On 287d6e5, we stopped to rm -rf debian/ on build_deb.sh, since now we have
prebuilt debian/ directory.
However, it might cause .deb build error when we modified debian package source,
since it never cleanup.
To prevent build error, we need to cleanup build/debian on reloc/build_deb.sh,
before extracting contents from relocatable package.
storage_proxy is never deinitialized, so it may have still used cdc_service
after its destructor was called.
This fixes the problem by cdc_service inheriting from
async_sharded_service and storage_proxy calling shared_from_this on
the service whenever it uses it.
cdc_service inherits from async_sharded_service and not simply from
enable_shared_from_this, because there might be other services that
cdc_service depends on. Assuming that these services are
deinitialized after cdc_service (as they should), i.e. after stop() is
called on cdc_service, making cdc_service async_sharded_service will
keep their deinitialization code from being called until all references
to cdc_service disappear (async_sharded_service keeps stop() from
returning until this happens).
Some more improvements should be possible through some refactoring:
1. Make augment_mutation_call a free function, not a member of
cdc_service: it doesn't need any state that cdc_service has.
db_context can be passed down from storage_proxy when it calls the
function.
2. Remove the storage_proxy -> cdc_service reference. storage_proxy
only needs augment_mutation_call, which would not be a part of the
service. This would also get rid of the proxy -> cdc -> proxy
reference cycle that we have now, and would allow storage_proxy to be
safely deinitialized after cdc_service.
3. Maybe we could even remove the cdc_service -> storage_proxy
reference. Is it really needed?
When a replacing node is in early boot up and is not in HIBERNATE sate
yet, if the node is killed by a user, the node will wrongly send a
shutdown message to other nodes. This is because UNKNOWN is not in
SILENT_SHUTDOWN_STATES, so in gossiper::do_stop_gossiping, the node will
send shutdown message. Other nodes in the cluster will call
storage_service::handle_state_normal for this node, since NORMAL and
SHUTDOWN status share the same status handler. As a result, other nodes
will incorrectly think the node is part of the cluster and the replace
operation is finished.
Such problem was seen in replace_node_no_hibernate_state_test dtest:
n1, n2 are in the cluster
n2 is dead
n3 is started to replace n2, but n3 is killed in the middle
n3 announces SHUTDOWN status wrongly
n1 runs storage_service::handle_state_normal for n3
n1 get tokens for n3 which is empty, because n3 hasn't gossip tokens yet
n1 skips update normal tokens for n3, but think n3 has replaced n2
n4 starts to replace n2
n4 checks the tokens for n2 in storage_service::join_token_ring (Cannot
replace token {} which does not exist!) or
storage_service::prepare_replacement_info (Cannot replace_address {}
because it doesn't exist in gossip)
To fix, we add UNKNOWN into SILENT_SHUTDOWN_STATES and avoid sending
shutdown message.
Tests: replace_address_test.py:TestReplaceAddress.replace_node_no_hibernate_state_test
Fixes: #6436
It seems that the following functions are never used, delete them:
* `function::has_reference_to`
* `functions::get_overload_count`
* `to_identifiers` in column_identifier.hh
* `single_column_relation::get_map_key`
Tests: unit(dev, debug)
Signed-off-by: Pavel Solodovnikov <pa.solodovnikov@scylladb.com>
Message-Id: <20200606115149.1770453-1-pa.solodovnikov@scylladb.com>
Merged pull request https://github.com/scylladb/scylla/pull/6516 from
Piotr Sarna:
This series adds error injection points to materialized view paths:
view update generation from staging sstables;
view building;
generating view updates from user writes.
This series comes with a corresponding dtest pull request which adds some
test cases based on error injection.
Fixes#6488
"
Now we generate dist/changelog on relocatable package generation time,
we cannot run '.rc' fixup on .deb package building time, need to do it
in debian_files_gen.py.
Also, we uses '_' in version number for some test version packages,
which does not supported in .deb packaging system, need to replaced
with '-'.
"
* syuu1228-debian_version_number_fix:
dist/debian: support version number containing '_'
dist/debian: move version number fixup to debian_files_gen.py
In current mutate_MV() code it's possible for a local endpoint
to become a target for a network operation. That's the source
of occasional `broken promise` benign error messages appearing,
since the mutation is actually applied locally, so there's no point
in creating a write response handler - the node will not send a response
to itself via network.
While at it, the code is deduplicated a little bit - with the paths
simplified, it's easier to ensure that a local endpoint is never
listed as a target for remote network operations.
Fixes#5459
Tests: unit(dev),
dtest(materialized_views_test.TestMaterializedViews.add_dc_during_mv_insert_test)
Overwriting a collection cell using timestamp T is a process with
following steps:
1. inserting a row marker (if applicable) with timestamp T;
2. writing a collection tombstone with timestamp T-1;
3. writing the new collection value with timestamp T.
Since CDC does clustering of the operations by timestamp, this
would result in 3 separate calls to `transform` (in case of
INSERT, or 2 - in the case of UPDATE), which seems excessive,
especially when pre-/postimage is enabled. This patch makes
collection tombstones being treated as if they had the same TS as
the base write and thus they are processed in one call to `transform`
(as long as TTLs are not used).
Also, `cdc_test` had to be updated in places that relied on former
splitting strategy.
Fixes#6084
For tombstone expiration to proceed correctly without the risk of resurrecting
data, the sstable set must be present.
Regular compaction and derivatives provide the sstable set, so they're able
to expire tombstones with no resurrection risk.
Resharding, on the other hand, can run on any shard, not necessarily on the
same shard that one of the input sstables belongs to, so it currently cannot
provide a sstable set for tombstone expiration to proceed safely.
That being said, let's only do expiration based on the presence of the set.
This makes room for the sstable set to be feeded to compaction via descriptor,
allowing even resharding to do expiration. Currently, compaction thinks that
sstable set can only come from the table, and that also needs to be changed
for further flexibility.
It's theoretically possible that a given resharding job will resurrect data if
a fully expired SSTable is resharded at a shard which it doesn't belong to.
Resharding will have no way to tell that expiring all that data will lead to
resurrection because the relevant SSTables are at different shards.
This is fixed by checking for fully expired sstables only on presence of
the sstable set.
Fixes#6600.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20200605200954.24696-1-raphaelsc@scylladb.com>
Now we generate dist/changelog on relocatable package generation time,
we cannot run '.rc' fixup on .deb package building time, need to do it
in debian_files_gen.py.
