Instead of announcing schema mutations the new function will return
them. The caller is responsible to announce them. To easy the transition
make the API optional. Statements that do not have it will use old
announce_migration() method.
Currently a keyspace mutation is included into schema mutation list just
before announcement. Move the inclusion to a separate function. It will
be used later when instead of announcing new schema the mutation array
will be returned.
When `check_and_repair_cdc_streams` encountered a node with status LEFT, Scylla
would throw. This behavior is fixed so that LEFT nodes are simply ignored.
Fixes#9771Closes#9778
There are several places that (still) use throwing b-tree .insert_before()
method and don't manage the inserted object lifetime. Some of those places
also leave the leaked rows_entry on the LRU delaying the assertion failure
by the time those entries get evicted (#9728)
To prevent such surprises in the future, the set removes the non-safe
inserters from the B-tree code. Actually most of this set is that removal
plus preparations for reviewability.
* xemul/br-rows-insertion-exception-safety-2:
btree: Earnestly discourage from insertion of plain references
row-cache: Handle exception (un)safety of rows_entry insertion
partition_snapshot_row_cursor: Shuffle ensure_result creation
mutation_partition: Use B-tree insertion sugar
tests: Make B-tree tests use unique-ptrs for insertion
The B-tree's insert_before() is throwing operation, its caller
must account for that. When the rows_entry's collection was
switched on B-tree all the risky places were fixed by ee9e1045,
but few places went under the radar.
In the cache_flat_mutation_reader there's a place where a C-pointer
is inserted into the tree, thus potentially leaking the entry.
In the partition_snapshot_row_cursor there are two places that not
only leak the entry, but also leave it in the LRU list. The latter
it quite nasty, because those entry can be evicted, eviction code
tries to get rows_entry iterator from "this", but the hook happens
to be unattached (because insertion threw) and fails the assert.
fixes: #9728
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Both places get the C-pointer on the freshly allocated rows_entry,
insert it where needed and return back the dereferenced pointer.
The C-pointer is going to become smart-pointer that would go out
of scope before return. This change prepares for that by constructing
the ensure_result from the iterator, that's returned from insertion
of the entry.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The B-tree insertion methods accept smart pointers and
automatically release the ownership after exception-risky
part is passed.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
In issue #9406 we noticed that a counter for BatchGetItem operations
was missing. When we fixed it, we added a test which checked this
counter - but only this counter. It was left as a TODO to test the rest
of the Alternator metrics, and this is what this patch does.
Here we add a comprehensive test for *all* of the operations supported
by Scylla and how they increase the appropriate operation counter.
With this test we discovered a new bug: the DescribeTimeToLive operation
incremented the UpdateTimeToLiveCounter :-( So in this patch we also
include a fix for that bug, and the new test verifies that it is fixed.
In addition to the operation counters, Alternator also has additional
metric and we also added tests for some of them - but not all. The
remaining untested metrics are listed in a TODO comment.
Message-Id: <20211206154727.1170112-1-nyh@scylladb.com>
Alternator's support for the DynamoDB API TTL features is experimental,
so if a user attempts to use one the TTL API requests, an error message
is returned that the experimental feature must be turned on first.
The message incorrectly said that the name of the experimental flag
to turn on is "alternator_ttl", with an underscore. But that's a type -
it should be "alternator-ttl" with a hyphen.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20211209183428.1336526-1-nyh@scylladb.com>
do_with_some_data runs a function in a seastar thread.
It needs to get() the future func returns rather
than propagating it.
This solves a secondary failure due to abandoned future
when the test case fails, as seen in
https://jenkins.scylladb.com/view/master/job/scylla-master/job/next/4254/artifact/testlog/x86_64_debug/database_test.snapshot_with_quarantine_works.381.log
```
test/boost/database_test.cc(903): fatal error: in "snapshot_with_quarantine_works": critical check expected.empty() has failed
WARN 2021-12-08 00:35:16,300 [shard 0] seastar - Exceptional future ignored: boost::execution_aborted, backtrace: 0x10935e50 0x16ff2d8d 0x16ff2a4d 0x16ff5033 0x16ff5ec2 0x162d4ce9 0x10a2bdb5 0x10a2bd24 0x10a54ca4 0x10a27cf3 0x10a22151 0x10a67c9d 0x10a67a78 0x163ac37e 0x163b29e9 0x163b7690 0x163b51c1 0x17c212df 0x17c1f097 0x17bf8b4c 0x17bf83f2 0x17bf82a2 0x17bf7d52 0x10f8bf5a 0x166db84b /lib64/libpthread.so.0+0x9298 /lib64/libc.so.6+0x100352
...
*** 1 abandoned failed future(s) detected
Failing the test because fail was requested by --fail-on-abandoned-failed-futures
```
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20211209174512.851945-1-bhalevy@scylladb.com>
In queries like:
```cql
SELECT * FROM t WHERE p = 0 AND c1 = 0 ORDER BY (c1 ASC, c2 ASC)
```
we can skip the requirement to specify ordering for `c1` column.
The `c1` column is restricted by an `EQ` restriction, so it can have
at most one value anyway, there is no need to sort.
This commit makes it possible to write just:
```cql
SELECT * FROM t WHERE p = 0 AND c1 = 0 ORDER BY (c2 ASC)
```
I reorganized the ordering code, I feel that it's now clearer and easier to understand.
It's possible to only introduce a small change to the existing code, but I feel like it becomes a bit too messy.
I tried it out on the [`orderby_disorder_small`](https://github.com/cvybhu/scylla/commits/orderby_disorder_small) branch.
The diff is a bit messy because I moved all ordering functions to one place,
it's better to read [select_statement.cc](https://github.com/cvybhu/scylla/blob/orderby_disorder/cql3/statements/select_statement.cc#L1495-L1658) lines 1495-1658 directly.
In the new code it would also be trivial to allow specifying columns in any order, we would just have to sort them.
For now I commented out the code needed to do that, because the point of this PR was to fix#2247.
Allowing this would require some more work changing the existing tests.
Fixes: #2247Closes#9518
* github.com:scylladb/scylla:
cql-pytest: Enable test for skipping eq restricted columns in order by
cql3: Allow to skip EQ restricted columns in ORDER BY
cql3: Add has_eq_restriction_on_column function
cql3: Reorganize orderings code
Seastar moved the read_entire_stream(), read_entire_stream_contiguous()
and skip_entire_stream() from the "httpd" namespace to the "util"
namespace. Using them with their old names causes deprecation warnings
when compiling alternator/server.cc.
This patch fixes the namespace (and adds the new include) to get rid of
the deprecation warnings.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20211209132759.1319420-1-nyh@scylladb.com>
"
Mutations are not guaranteed to come in the order of their timestamps.
If there is an expired tombstone in the sstable and a repair inserts old
data into memtable, the compaction would not consider memtable data and
purge the tombstone leading to data resurrection. The solution is to
disallow purging tombstones newer than min memtable timestamp. If there
are no memtables, max timestamp is used.
"
* 'check-memtable-at-compact-tombstone-discard/v2' of github.com:mikolajsieluzycki/scylla:
table: Prevent resurrecting data from memtable on compaction
table: Add min_memtable_timestamp function to table
Right now we do not really have any parallelism in the alternator
TTL service, but in order to be future-proof, a semaphore
is instantiated to ensure that we only handle 1 page of a scan
at a time, regardless of how many tables are served.
This commit also removes the FIXME regarding the service permit
- using an empty permit is a conscious decision, because the
parallelism is limited by other means (see above).
Tests: unit(release)
Message-Id: <b5f0c94f1afbead1f940a210911cc05f70900dcd.1638990637.git.sarna@scylladb.com>
This test was marked as xfail, but now the functionality it tests has been implemented.
In my opinion the expected error message makes no sense, the message was:
"Order by currently only supports the ordering of columns following their declared order in the PRIMARY KEY"
In cases where there was missing restriction on one column.
This has been changed to:
"Unsupported order by relation - column {} doesn't have an ordering or EQ relation."
Because of that I had to modify the test to accept messages from both Scylla and Cassandra.
The expected error message pattern is now "rder by", because that's the largest common part.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Mutations are not guaranteed to come in the order of their timestamps.
If there is an expired tombstone in the sstable and a repair inserts old
data into memtable, the compaction would not consider memtable data and
purge the tombstone leading to data resurrection. The solution is to
disallow purging tombstones newer than min memtable timestamp.
In queries like:
SELECT * FROM t WHERE p = 0 AND c1 = 0 ORDER BY (c1 ASC, c2 ASC)
we can skip the requirement to specify ordering for c1 column.
The c1 column is restricted by an EQ restriction, so it can have
only one value anyway, there is no need to sort.
This commit makes it possible to write just:
SELECT * FROM t WHERE p = 0 AND c1 = 0 ORDER BY (c2 ASC)
Fixes: #2247
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Adds a function that checks whether a given expression has eq restrction
on the specified column.
It finds restrictions like
col = ...
or
(col, col2) = ...
IN restrictions don't count, they aren't EQ restrictions
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
Reorganized the code that handles column ordering (ASC or DESC).
I feel that it's now clearer and easier to understand.
Added an enum that describes column ordering.
It has two possible values: ascending or descending.
It used to be a bool that was sometimes called 'reversed',
which could mean multiple things.
Instead of column.type->is_reversed() != <ordering bool>
there is now a function called are_column_select_results_reversed.
Split checking if ordering is reversed and verifying whether it's correct into two functions.
Before all of this was done by is_reversed()
This is a preparation to later allow skipping ORDER BY restrictions on some columns.
Adding this to the existing code caused it to get quite complex,
but this new version is better suited for the task.
The diff is a bit messy because I moved all ordering functions to one place,
it's better to read select_statement.cc lines 1495-1651 directly.
Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
The description in protocols.md of the Redis protocol server in Scylla
explains how its port can be configured, but not how the listening IP
address can be configured. It turns out that the same "rpc_address" that
controls CQL's and Thrift's IP address also applies to Redis. So let's
document that.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20211208160206.1290916-1-nyh@scylladb.com>
This series fixes a couple issues around generating and handling of no_such_keyspace and no_such_column_family exceptions.
First, it removes std::throw_with_nested around their throw sites in the respective database::find_* functions.
Fixes#9753
And then, it introduces a `validate_tables` helper in api/storage_service.cc that generates a `bad_param_exception` in order to set the correct http response status if a non-existing table name is provided in the `cf` http request parameter.
Fixes#9754
The series also adds a test for the REST API under test/rest_api that verifies the storage_service enable/disable auto_compaction api and checks the error codes for non-existing keyspace or table.
Test: unit(dev)
Closes#9755
* github.com:scylladb/scylla:
api: storage_service: add parse_tables
database: un-nest no_such_keyspace and no_such_column_family exceptions
database: throw internal error when failing uuid returned by find_uuid
database: find_uuid: throw no_such_column_family exception if ks/cf were not found
test: rest_api: add storage_service test
test: add basic rest api test
test: cql-pytest: wait for rest api when starting scylla
Splits and validate the cf parameter, containing an optional
comma-separated list of table names.
If any table is not found and a no_such_column_family
exception is thrown, wrap it in a `bad_param_exception`
so it will translate to `reply::status_type::bad_request`
rather than `reply::status_type::internal_server_error`.
With that, hide the split_cf function from api/api.hh
since it was used only from api/storage_service
and new use sites should use validate_tables instead.
Fixes#9754
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
These were thrown in the respective database::find_*
function as nested exception since
d3fe0c5182.
Wrapping them in nested exceptions just makes it
harder to figure out and work with and apprently serves
no purpose.
Without these nested_exception we can correctly detect
internal errors when synchronously failing to find
a uuid returned by find_uuid(ks, cf).
Fixes#9753
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
find_uuid returns a uuid found for ks_name.table_name.
In some cases, we immediately and synchronously use that
uuid to lookup other information like the table&
or the schema. Failing to find that uuid indicates
an internal error when no preemption is possible.
Note that yielding could allow deletion of the table
to sneak in and invalidate the uuis.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Rather than masquerading all errors as std::out_of_range("")
convert only the std::out_of_range error from _ks_cf_to_uuid.at()
to no_such_column_family(ks, cf). That relieves all callers of
fund_uuid from doing that conversion themselves.
For example, get_uuid in api/column_family now only deals with converting
no_such_column_family to bad_param_exception, as it needs to do
at the api level, rather than generating a similar error from scratch.
Other call sites required no intervention.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
FIXME: negative tests for not-found tables
should result in a requests.codes.bad_request
but currently result in requests.codes.internal_server_error.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
This patch addresses yet another FIXME from alternator/ttl.cc.
Namely, scans are now started from a random, owned token range
instead of always starting with the first range.
This mechanism is expected to reduce the probability of some
ranges being starved when the scanning process is often restarted,
e.g. due to nodes failing.
Should the mechanism prove insufficient for some users, a more complete
solution is to regularly persist the state of the scanning process
in a table (distributed if we want to allow other nodes to pick up
from where a dead node left off), but that induces overhead.
Tests: unit(release) (including a long loop over the ttl pytest)
Message-Id: <7fc3f6525ceb69725c41de10d0fb6b16188349e3.1638387924.git.sarna@scylladb.com>
Message-Id: <db198e743ca9ed1e5cc659e73da342fbce2c882a.1638473143.git.sarna@scylladb.com>
Some of the tests, like nodetool.py, use the scylla REST API.
Add a check_rest_api function that queries http://<node_addr>:10000/
that is served once scylla starts listening on the API port
and call it via run.wait_for_services.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
On my local machine, a 3 second deadline proved to cause flakiness
of test_ttl_expiration case, because its execution time is just
around 3 seconds.
This patch addresse the problem by bumping the local timeout to 10
(and 15 for test_ttl_expiration_long, since it's dangerously near
the 10 second deadline on my machine as well).
Moreover, some test cases short-circuited once they detected that
all needed items expired, but other ones lacked it and always used
their full time slot. Since 10 seconds is a little too long for
a single test case, even one marked with --veryslow, this patch
also adds a couple of other short-circuits.
One exception is test_ttl_expiration_hash_wrong_type, which actually
depends on the fact that we should wait for the whole loop to finish.
Since this case was never flaky for me with the 3 second timeout,
it's left as is.
Theoretically, test_ttl_expiration also kind of depends on checking
the condition more than once (because the TTL of one of the values
is bumped on each iteration), but empirical evidence shows that
multiple iterations always occur in this test case anyway - for
me, it always spinned at least 3 times.
Tests: unit(release)
Message-Id: <a0a479929dac37daace744e0a970567a8aa3b518.1638431933.git.sarna@scylladb.com>
if fill_buffer() is called after EOS, underlying reader will
be fast forwarded to a range pointed to by an invalid iterator,
so producing incorrect results.
fill_buffer() is changed to return early if EOS was found,
meaning that underlying reader already fast forwarded to
all ranges managed by multi_range_reader.
Usually, consume facilities check for EOS, before calling
fill_buffer() but most reader impl check for EOS to avoid
correctness issues. Let's do the same here.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20211208131423.31612-1-raphaelsc@scylladb.com>
* seastar f8a038a0a2...8d15e8e67a (21):
> core/program_options: preserve defaultness of CLI arguments
> log: Silence logger when logging
> Include the core/loop.hh header inside when_all.hh header
> http: Fix deprecated wrappers
> foreign_ptr: Add concept
> util: file: add read_entire_file
> short_streams: move to util
> Revert "Merge: file: util: add read_entire_file utilities"
> foreign_ptr: declare destroy as a static method
> Merge: file: util: add read_entire_file utilities
> Merge "output_stream: handle close failure" from Benny
> net: bring local_address() to seastar::connected_socket.
> Merge "Allow programatically configuring seastar" from Botond
> Merge 'core: clean up memory metric definitions' from John Spray
> Add PopOS to debian list in install-dependencies.sh
> Merge "make shared_mutex functions exception safe and noexcept" from Benny
> on_internal_error: set_abort_on_internal_error: return current state
> Implementation of iterator-range version of when_any
> net: mark functions returning ethernet_address noexcept
> net: ethernet_address: mark functions noexcept
> shared_mutex: mark wake and unlock methods noexcept
Contains patch from Botond Dénes <bdenes@scylladb.com>:
db/config: configure logging based on app_template::seastar_options
Scylla has its own config file which supports configuring aspects of
logging, in addition to the built-in CLI logging options. When applying
this configuration, the CLI provided option values have priority over
the ones coming from the option file. To implement this scylla currently
reads CLI options belonging to seastar from the boost program options
variable map. The internal representation of CLI options however do not
constitute an API of seastar and are thus subject to change (even if
unlikely). This patch moves away from this practice and uses the new
shiny C++ api: `app_template::seastar_options` to obtain the current
logging options.
This series wires up the schema state machine to process raft commands
and transfer snapshots. The series assumes that raft group zero is used
for schema transfer only and that single raft command contains single
schema change in a form of canonical_mutation array. Both assumptions
may change in which case the code will be changed accordingly, but we
need to start somewhere.
* scylla-dev/gleb/schema-raft-sm-v2:
schema raft sm: request schema sync on schema_state_machine snapshot transfer
raft service: delegate snapshot transfer to a state machine implementation
schema raft sm: pass migration manager to schema_raft_state_machine and merge schema on apply()
Add a short test verifying that Alternator responds with the correct
error code (UnknownOperationException) when receiving an unknown or
unsupported operation.
The test passes on both AWS and Alternator, confirming that the behavior
is the same.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20211206125710.1153008-1-nyh@scylladb.com>
