sstables_manager will soon be closed asynhronously, with a future-returning
close() function. To prepare for that, make the following changes
- replace test_sstables_manager with an sstables_manager obtained from test_env
- drop unneeded calls to await_background_jobs()
These changes allow lifetime management of the sstables_manager used
in the tests to be centralized in test_env.
sstables_manager will soon be closed asynhronously, with a future-returning
close() function. To prepare for that, make the following changes
- acquire a test_env with test_env::do_with() (or the sharded variant)
- change the sstable_from_existing_file function to be a functor that
works with either cql_test_env or test_env (as this is what individual
tests want); drop use of test_sstables_manager
- change new_sstable() to accept a test_env instead of using test_sstables_manager
- replace test_sstables_manager with an sstables_manager obtained from test_env
These changes allow lifetime management of the sstables_manager used
in the tests to be centralized in test_env.
sstables_manager will soon be closed asynhronously, with a future-returning
close() function. To prepare for that, make the following changes
- replace on-stack test_env with test_env::do_with()
- use the variant of column_family_for_tests that accepts an sstables_manager
- replace test_sstables_manager with an sstables_manager obtained from test_env
These changes allow lifetime management of the sstables_manager used
in the tests to be centralized in test_env.
Since test_env now calls await_background_jobs on termination, those
calls are dropped.
Use the sstables_manager from test_env. Use do_with_async() to create the test_env,
to allow for proper closing.
Since do_with_async() also takes care of await_background_jobs(), remove that too.
test_sstables_manager is going away, so replace it by test_env::manager().
column_family_test_config() has an implicit reference to test_sstables_manager,
so pass test_env::manager() as a parameter.
Calls to await_background_jobs() are removed, since test_env::stop() performs
the same task.
The large rows tests are special, since they use a custom sstables_manager,
so instead of using a test_env, they just close their local sstables_manager.
Acquire a test_env and extract an sstables_manager from that, passing it
to column_familty_test_config, in preparation for losing the default
constructor of column_familty_test_config.
Some tests need a sharded sstables_manager, prepare for that by
adding a stop() method and helpers for creating a sharded service.
Since test_env doesn't yet contain its own sstable_manager, this
can't be used in real life yet.
sstables::test_env needs to be properly closed (and will soon need it
even more). Use test_env::do_with_async() to do that. Removed
await_background_jobs(), which is now done by test_env::close().
A test_env contains an sstables_manager, which will soon have a close() method.
As such, it can no longer be a temporary. Switch to using test_env::do_with_async().
As a bonus, test_env::do_with_async() performs await_background_jobs() for us, so
we can drop it from the call sites.
A test_env contains an sstables_manager, which will soon have a close() method.
As such, it can no longer be a temporary. Switch to using test_env::do_with_async().
A test_env contains an sstables_manager, which will soon have a close() method.
As such, it can no longer be a temporary. Switch to using test_env::do_with_async().
As a bonus, test_env::do_with_async() performs await_background_jobs() for us, so
we can drop it from the call sites.
Since we're dropping test_sstables_manager, we'll require callers to pass it
to column_family_test_config, so provide overloads that accept it.
The original overloads (that don't accept an sstables_manager) remain for
the transition period.
Some tests are now referencing the global test_sstables_manager,
which we plan to remove. Add test_env::manager() as a way to
reference the sstables_manager that the test_env contains.
The pattern
return function_returning_a_future().get();
is legal, but confusing. It returns an unexpected std::tuple<>. Here,
it doesn't do any harm, but if we try to coerce the surrounding code
into a signature (void ()), then that will fail.
Remove the unneeded and unexpected return.
Drop references to a table's sstables when stopping it, so that
the sstable_manager can start deleting it. This includes staging
sstables.
Although the table is no longer in use at this time, maintain
cache synchronity by calling row_cache::invalidate() (this also
has the benefit of avoiding a stall in row_cache's destructor).
We also refresh the cache's view of the sstable set to drop
the cache's references.
Take the gate in table::query() so that stop() waits for queries. The gate
is already waited for in table::stop().
This allows us to know we are no longer using the table's sstables in table::stop().
sstables_manager is going to take charge of its sstables lifetimes,
so it will need a close() to wait until sstables are deleted.
This patch adds sstables_manager::close() so that the surrounding
infrastructure can be wired to call it. Once that's done, we can
make it do the waiting.
The make_sstable_directory_for*() functions run in a thread, and
call functions that run in a thread, but return a future. This
more or less works but is a dangerous construct that can fail.
Fix by returning a regular value.
Stopping a table will soon close its sstables; so the next check will fail
as the number of sstables for the table will be zero.
Reorder the stop() call to make it safe.
We don't need the stop() for the check, since the previous loop made sure
compactions completed.
test_view_update_generator_register_semaphore_unit_leak creates a continuation chain
inside a timer, but does not wait for it. This can result in part of the chain
being executed after its captures have been destroyed.
This is unlikely to happen since the timer fires only if the test fails, and
tests never fail (at least in the way that one expects).
Fix by waiting for that future to complete before exiting the thread.
test_view_update_generator_register_semaphore_unit_leak uses a thread function
in do_with_cql_env(), even though the latter doesn't promise a thread and
accepts a regular function-returning-a-future. It happens to work because the
function happens to be called in a thread, but this isn't guaranteed.
Switch to do_with_cql_env, which guarantees a thread context.
sstable_manager will soon wait for all sstables under its
control to be deleted (if so marked), but that can't happen
if someone is holding on to references to those sstables.
To allow sstables_manager::stop() to work, drop remaining
queued work when terminating.
As the test test_streams_closed_read confirmed, when a stream shard is
closed, GetRecords should not return a NextShardIterator at all.
Before this patch we wrongly returned an empty string for it.
Before this patch, several Alternator Stream tests (in test_streams.py)
failed when running against a multi-node Scylla cluster. The reason is as
follows: As a multi-node cluster boots and more and more nodes enter the
cluster, the cluster changes its mind about the token ownership, and
therefore the list of stream shards changes. By the time we have the full
cluster, a bunch of shards were created and closed without any data yet.
All the tests will see these closed shards, and need to understand them.
The fetch_more() utility function correctly assumed that a closed shard
does not return a NextShardIterator, and got confused by the empty string
we used to return.
Now that closed shards can return responses without NextShardIterator,
we also needed to fix in this patch a couple of tests which wrongly assumed
this can't happen. These tests did not fail on DynamoDB because unlike in
Scylla, DynamoDB does not have any closed shards in normal tests which
do not specifically cause them (only test_streams_closed_read).
We also need to fix test_streams_closed_read to get rid of an unnecessary
assumption: It currently assumes that when we read the very last item in
a closed shard is read, the end-of-shard is immediately signaled (i.e.,
NextShardIterator is not returned). Although DynamoDB does in fact do this,
it is also perfectly legal for Alternator's implementation to return the
last item with a new NextShardIterator - and only when the client reads
from that iterator, we finally return the signal the end of the shard.
Fixes#7237.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200922082529.511199-1-nyh@scylladb.com>
The classes touche private data of each other for no real
reason. Putting the interaction behind API makes it easier
to track the usage.
* xemul/br-unfriends-in-row-cache-2:
row cache: Unfriend classes from each other
rows_entry: Move container/hooks types declarations
rows_entry: Simplify LRU unlink
mutation_partition: Define .replace_with method for rows_entry
mutation_partition: Use rows_entry::apply_monotonically
Merged pull request https://github.com/scylladb/scylla/pull/7264 by
Avi Kivity (29 commits):
This series fixes issues found by clang. Most are real issues that gcc just
doesn't find, a few are due to clang lagging behind on some C++ updates.
See individual explanations in patches.
The series is not sufficient to build with clang; it just addresses the
simple problems. Two larger problems remain: clang isn't able to compile
std::ranges (not clear yet whether this is a libstdc++ problem or a clang
problem) and clang can't capture structured binding variables (due to
lagging behind on the standard).
The motivation for building with clang is gaining access to a working
implementation of coroutines and modules.
This series compiles with gcc and the unit tests pass.
Currently in all cases we first deduct the to-be-consumed resources,
then construct the `reader_resources` class to protect it (release it on
destruction). This is error prone as it relies on no exception being
thrown while constructing the `reader_resources`. Albeit the
`reader_resources` constructor is `noexcept` right now this might change
in the future and as the call sites relying on this are disconnected
from the declaration, the one modifying them might not notice.
To make this safe going forward, make the `reader_resources` a true RAII
class, consuming the units in its constructor and releasing them in its
destructor.
Fixes: #7256
Tests: unit(dev)
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <20200922150625.1253798-1-bdenes@scylladb.com>
We have templates for multiprecision_int for both sides of the operator,
for example:
template <typename T>
bool operator==(const T& x) const
and
template <typename T>
friend bool operator==(const T& x, const multiprecision_int& y)
Clang considers them equally satisfying when both operands are
multiprecision_int, so provide a disambiguating overload.
Clang dislikes forward-declared functions returning auto, so declare the
type up front. Functions returning auto are a readability problem
anyway.
To solve a circular dependency problem (get_local_injector() ->
error_injection<> -> get_local_injector()), which is further compounded
by problems in using template specializations before they are defined
(which is forbidden), the storage for get_local_injector() was moved
to error_injection<>, and get_local_injector() is just an accessor.
After this, error_injection<> does not depend on get_local_injector().
new_sstable is defined as a template, and later used in a context
that requires an object. Somehow gcc uses an instantiation with
an empty template parameter list, but I don't think it's right,
and clang refuses.
Since the template is gratuitous anyway, just make it a regular
function.
This patch adds a test, test_streams_closed_read, which reproduces
two issues in Alternator Streams, regarding the behavior of *closed*
stream shards:
Refs #7239: After streaming is disabled, the stream should still be readable,
it's just that all its shards are now "closed".
Refs #7237: When reaching the end of a closed shard, NextShardIterator should
be missing. Not set to an empty string as we do today.
The test passes on DynamoDB, and xfails on Alterator, and should continue to
do so until both issues are fixed.
This patch changes the implementation of the disable_stream() function.
This function was never actually used by the existing code, and now that
I wanted to use it, I discovered it didn't work as expected and had to fix it.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200915134643.236273-1-nyh@scylladb.com>
The gossiper verbs are registered in two places -- start_gossiping
and do_shadow_round(). And unregistered in one -- stop_gossiping
iff the start took place. Respectively, there's a chance that after
a shadow round scylla exits without starting gossiping thus leaving
verbs armed.
Fix by unregistering verbs on stop if they are still registered.
fixes: #7262
tests: manual(start, abort start after shadow round), unit(dev)
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Message-Id: <20200921140357.24495-1-xemul@scylladb.com>
Before updating the _last_[cp]key (for subsequent .fetch_page())
the pager checks is 'if the pager is not exhausted OR the result
has data'.
The check seems broken: if the pager is not exhausted, but the
result is empty the call for keys will unconditionally try to
reference the last element from empty vector. The not exhausted
condition for empty result can happen if the short_read is set,
which, in turn, unconditionally happens upon meeting partition
end when visiting the partition with result builder.
The correct check should be 'if the pager is not exhausted AND
the result has data': the _last_[pc]key-s should be taken for
continuation (not exhausted), but can be taken if the result is
not empty (has data).
fixes: #7263
tests: unit(dev), but tests don't trigger this corner case
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Message-Id: <20200921124329.21209-1-xemul@scylladb.com>
The .get_last_partition_and_clustering_key() method gets
the last partition from the on-board vector of partitions.
The vector in question is assumed not to be empty, but if
this assumption breaks, the result will look like memory
corruption (docs say that accessing empty's vector back()
results in undefined behavior).
tests: unit(dev)
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Message-Id: <20200921122948.20585-1-xemul@scylladb.com>
