All the places in server.cc where we constructed an api_error with inline
strings now use api_error factory functions - we needed to add a few more.
Interestingly, we had a wrong type string for "Internal Server Error",
which we fix in this patch. We wrote the type string like that - with spaces -
because this is how it was listed in the DynamoDB documentation at
https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Programming.Errors.html
But this was in fact wrong, and it should be without spaces:
"InternalServerError". The botocore library (for example) recognizes it
this way, and this string can also be seen in other online DynamoDB examples.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
In the patch "Add exception overloads for Dynamo types", Alternator's single
api_error exception type was replaced by a more complex hierarchy of types.
The implementation was not only longer and more complex to understand -
I believe it also negated an important observation:
The "api_error" exception type is special. It is not an exception created
by code for other code. It is not meant to be caught in Alternator code.
Instead, it is supposed to contain an error message created for the *user*,
containing one of the few supported exception exception "names" described
in the DynamoDB documentation, and a user-readable text message. Throwing
such an exception in Alternator code means the thrower wants the request
to abort immediately, and this message to reach the user. These exceptions
are not designed to be caught in Alternator code. Code should use other
exceptions - or alternatives to exceptions (e.g., std::optional) for
problems that should be handled before returning a different error to the
user. Moreover, "api_error" isn't just thrown as an exception - it can
also be returned-by-value in a executor::request_return_type) - which is
another reason why it should not be subclassed.
For these reasons, I believe we should have a single api_error type, and
it's wrong to subclass it. So in this patch I am reverting the subclasses
and template added in the aforementioned patch.
Still, one correct observation made in that patch was that it is
inconvenient to type in DynamoDB exception names (no help from the editor
in completing those strings) and also error-prone. In this patch we
propse a different - simpler - solution to the same problem:
We add trivial factory functions, e.g., api_error::validation(std::string)
as a shortcut to api_error("ValidationException"). The new implementation
is easy to understand, and also more self explanatory to readers:
It is now clear that "api_error::validation()" is actually a user-visible
"api_error", something which was obscured by the name validation_exception()
used before this patch.
Finally, this patch also improves the comment in error.hh explaining the
purpose of api_error and the fact it can be returned or thrown. The fact
it should not be subclassed is legislated with a "finally". There is also
no point of this class inheriting from std::exception or having virtual
functions, or an empty constructor - so all these are dropped as well.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Let each submodule be responsible for its own dependencies, and
call the submodule's dependency installation script.
Reviewed-by: Piotr Jastrzebski <piotr@scylladb.com>
Reviewed-by: Takuya ASADA <syuu@scylladb.com>
tools/java and tools/jmx have their own relocatable packages (and rpm/deb),
so they should not be part of the main relocatable package.
Enforce this by enabling the filter parameter in reloc_add, and passing
a filter that excludes tools/java and tools/jmx.
If ring_delay == 0, something fishy is going on, e.g. single-node tests
are being performed. In this case we want the CDC generation to start
operating immediately. There is no need to wait until it propagates to
the cluster.
You should not use ring_delay == 0 in production.
Fixes https://github.com/scylladb/scylla/issues/6864.
"
This adds a '--io-setup N' command line option, which users can pass to
specify whether they want to run the "scylla_io_setup" script or not.
This is useful if users want to specify I/O settings themselves in
environments such as Kubernetes, where running "iotune" is problematic.
While at it, add the same option to "scylla_setup" to keep the interface
between that script and Docker consistent.
Fixes#6587
"
* penberg-penberg/docker-no-io-setup:
scylla_setup: Add '--io-setup ENABLE' command line option
dist/docker: Add '--io-setup ENABLE' command line option
* seastar 4a99d56453...02ad74fa7d (5):
> TLS: Use "known" (precalculated) DH parameters if available
> tutorial: fix advanced service_loop examples
> tutorial: further fix service_loop example text
> linux-aio: make the RWF_NOWAIT support work again
> locking_test: Fix a use after return
To make the "scylla_setup" interface similar to Docker image, let's add
a "--io-setup ENABLE" command line option. The old "--no-io-setup"
option is retained for compatibility.
This adds a '--io-setup N' command line option, which users can pass to
specify whether they want to run the "scylla_io_setup" script or not.
This is useful if users want to specify I/O settings themselves in
environments such as Kubernetes, where running "iotune" is problematic.
Fixes#6587
We recently saw a weird log message:
WARN 2020-07-19 10:22:46,678 [shard 0] repair - repair id [id=4,
uuid=0b1092a1-061f-4691-b0ac-547b281ef09d] failed: std::runtime_error
({shard 0: fmt::v6::format_error (invalid type specifier), shard 1:
fmt::v6::format_error (invalid type specifier)})
It turned out we have:
throw std::runtime_error(format("repair id {:d} on shard {:d} failed to
repair {:d} sub ranges", id, shard, nr_failed_ranges));
in the code, but we changed the id from integer to repair_uniq_id class.
We do not really need to specify the format specifiers for numbers.
Fixes#6874
Staging SSTables can be incorrectly added or removed from the backlog tracker,
after an ALTER TABLE or TRUNCATE, because the add and removal don't take
into account if the SSTable requires view building, so a Staging SSTable can
be added to the tracker after a ALTER table, or removed after a TRUNCATE,
even though not added previously, potentially causing the backlog to
become negative.
Fixes#6798.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20200716180737.944269-1-raphaelsc@scylladb.com>
test.py passes the "--junit-xml" option to test/alternator/run, which passes
this option to pytest to get an xunit-format summary of the test results.
However, unfortunately until very recent versions (which aren't yet in Linux
distributions), pytest defaulted to a non-standard xunit format which tools
like Jenkins couldn't properly parse. The more standard format can be chosen
by passing the option "-o junit_family=xunit2", so this is what we do in
this patch.
Fixes#6767 (hopefully).
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200719203414.985340-1-nyh@scylladb.com>
"
The set's goal is to reduce the indirect fanout of 3 headers only,
but likely affects more. The measured improvement rates are
flat_mutation_reader.hh: -80%
mutation.hh : -70%
mutation_partition.hh : -20%
tests: dev-build, 'checkheaders' for changed headers (the tree-wide
fails on master)
"
* 'br-debloat-mutation-headers' of https://github.com/xemul/scylla:
headers:: Remove flat_mutation_reader.hh from several other headers
migration_manager: Remove db/schema_tables.hh inclustion into header
storage_proxy: Remove frozen_mutation.hh inclustion
storage_proxy: Move paxos/*.hh inclusions from .hh to .cc
storage_proxy: Move hint_wrapper from .hh to .cc
headers: Remove mutation.hh from trace_state.hh
schema_mutations
When upgrading from a version that lacks some schema features,
during the transition, when we have a mixed cluster. Schema digests
are calculated without taking into account the mixed cluster supported
features. Every node calculate the digest as if the whole cluster supports
its supported features.
Scylla already has a mechanism of redaction to the lowest common
denominator, but it haven't been used in this context.
This commit is using the redaction mechanism when calculating the digest on
the newly added table so it will match the supported features of the
whole cluster.
Tests: Manual upgrading - upgraded to a version with an additional
feature and additional schema column and validated that the digest
of the tables schema is identical on every node on the mixed cluster.
All they can live with forward declaration of the f._m._r. plus a
seastar header in commitlog code.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The schema_tables.hh -> migration_manager.hh couple seems to work as one
of "single header for everyhing" creating big blot for many seemingly
unrelated .hh's.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
It's only used there, but requires mutation_query.hh, which can thus be
removed from storage_proxy.hh
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
In test_streams.py, we had the line:
assert desc['StreamDescription'].get('StreamLabel')
In Alternator, the 'StreamLabel' attribute is missing, which the author of
this test probably thought would cause this test to fail (which is expected,
the test is marked with "xfail"). However, my version of pytest actually
doesn't like that assert is given a value instead of a comparison, and we
get the warning message:
PytestAssertRewriteWarning: asserting the value None, please use "assert is None"
I think that the nicest replacement for this line is
assert 'StreamLabel' in desc['StreamDescription']
This is very readable, "pythonic", and checks the right thing - it checks
that the JSON must include the 'StreamLabel' item, as the get() assertion
was supposed to have been doing.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200716124621.906473-1-nyh@scylladb.com>
If external is true, _u.ptr is not null. An empty managed_bytes uses
the internal representation.
The current code looks scary, since it seems possible that backref
would still point to the old location, which would invite corruption
when the reclaimer runs.
Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
Reviewed-by: Benny Halevy <bhalevy@scylladb.com>
Message-Id: <20200716233124.521796-1-espindola@scylladb.com>
* seastar 0fe32ec59...4a99d5645 (3):
> httpd: Don't warn on ECONNABORTED
> httpd: Avoid calling future::then twice on the same future
Fixes#6709.
> futures: Add a test for a broken promise in repeat
Ninja has a special pool called console that causes programs in that
pool to output directly to the console instead of being logged. By
putting test.py in it is now possible to run just
$ ninja dev-test
And see the test.py output while it is running.
Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
Message-Id: <20200716204048.452082-1-espindola@scylladb.com>
"
Fix#6825 by explicitly distinguishing single- from multi-column expressions in AST.
Tests: unit (dev), dtest secondary_indexes_test.py (dev)
"
* dekimir-single-multiple-ast:
cql3/restrictions: Separate AST for single column
cql3/restrictions: Single-column helper functions
Corresponding overload of `storage_proxy::mutate_locally`
was hardcoded to pass `db::commitlog::force_sync::no` to the
`database::apply`. Unhardcode it and substitute `force_sync::no`
to all existing call sites (as it were before).
`force_sync::yes` will be used later for paxos learn writes
when trying to apply mutations upgraded from an obsolete
schema version (similar to the current case when applying
locally a `frozen_mutation` stored in accepted proposal).
Tests: unit(dev)
Signed-off-by: Pavel Solodovnikov <pa.solodovnikov@scylladb.com>
Message-Id: <20200716124915.464789-1-pa.solodovnikov@scylladb.com>
"
We'd like to use the same uuid both for printing compaction log
messages and to update compaction_history.
Generate one when starting compaction and keep it in
compaction_info. Then use it by convention in all
compaction log messages, along with compaction type,
and keyspace.table information. Finally, use the
same uuid to update compaction_history.
Fixes#6840
"
* tag 'compaction-uuid-v1' of github.com:bhalevy/scylla:
compaction: print uuid in log messages
compaction: report_(start|finish): just return description
compaction: move compaction uuid generation to compaction_info
The targets {dev|debug|release}-test run all unit tests, including
alternator/run. But this test requires the Scylla executable, which
wasn't among the dependencies. Fix it by adding build/$mode/scylla to
the dependency list.
Fixes#6855.
Tests: `ninja dev-test` after removing build/dev/scylla
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
It is legal for a user to create a table with name that has a
_scylla_cdc_log suffix. In such case, the table won't be treated as a
cdc log table, and does not require a corresponding base table to exist.
During refactoring done as a part of initial implemetation of of
Alternator streams (#6694), `is_log_for_some_table` started throwing
when trying to check a name like `X_scylla_cdc_log` when there was no
table with name `X`. Previously, it just returned false.
The exception originates inside `get_base_table`, which tries to return
the base table schema, not checking for its existence - which may throw.
It makes more sense for this function to return nullptr in such case (it
already does when provided log table name does not have the cdc log
suffix), so this patch adds an explicit check and returns nullptr when
necessary.
A similar oversight happened before (see #5987), so this patch also adds
a comment which explains why existence of `X_scylla_cdc_log` does not
imply existence of `X`.
Fixes: #6852
Refs: #5724, #5987
By convention, print the following information
in all compaction log messages:
[{compaction.type} {keyspace}.{table} {compaction.uuid}]
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Rather than logging the message in the virtual callee method
just return a string description and make the logger call in
the common caller.
1. There is no need to do the logger call in the callee,
it is simpler to format the log message in the the caller
and just retrieve the per-compaction-type description.
2. Prepare to centrally print the compaction uuid.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
We'd like to use the same uuid both for printing compaction log
messages and to update compaction_history.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Existing AST assumes the single-column expression is a special case of
multi-column expressions, so it cannot distinguish `c=(0)` from
`(c)=(0)`. This leads to incorrect behaviour and dtest failures. Fix
it by separating the two cases explicitly in the AST representation.
Modify AST-creation code to create different AST for single- and
multi-column expressions.
Modify AST-consuming code to handle column_name separately from
vector<column_name>. Drop code relying on cardinality testing to
distinguisn single-column cases.
Add a new unit test for `c=(0)`.
Fixes#6825.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
This commit is separated out for ease of review. It introduces some
functions that subsequent commits will use, thereby reducing diff
complexity of those subsequent commits. Because the new functions
aren't invoked anywhere, they are guarded by `#if 0` to avoid
unused-function errors.
The new functions perform the same work as their existing namesakes,
but assuming single-column expressions. The old versions continue to
try handling single-column as a special case of multi-column,
remaining unable to distinguish between `c = (1)` and `(c) = (1)`.
This will change in the next commit, which will drop attempts to
handle single-column cases from existing functions.
Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
Currently, repair uses an integer to identify a repair job. The repair
id starts from 1 since node restart. As a result, different repair jobs
will have same id across restart.
To make the id more unique across restart, we can use an uuid in
addition to the integer id. We can not drop the use of the integer id
completely since the http api and nodetool use it.
Fixes#6786
The markdown syntax in the contribution section is incorrect, which is
why the links appear on the same line.
Improve the contribution section by making it more explicit what the
links are about.
Message-Id: <20200714070716.143768-1-penberg@scylladb.com>
This test usually fails, with the following error. Marking it "xfail" until
we can get to the bottom of this.
dynamodb = dynamodb.ServiceResource()
dynamodbstreams = <botocore.client.DynamoDBStreams object at 0x7fa91e72de80>
def test_get_records(dynamodb, dynamodbstreams):
# TODO: add tests for storage/transactionable variations and global/local index
with create_stream_test_table(dynamodb, StreamViewType='NEW_AND_OLD_IMAGES') as table:
arn = wait_for_active_stream(dynamodbstreams, table)
p = 'piglet'
c = 'ninja'
val = 'lucifers'
val2 = 'flowers'
> table.put_item(Item={'p': p, 'c': c, 'a1': val, 'a2': val2})
test_streams.py:316:
...
E botocore.exceptions.ClientError: An error occurred (Internal Server Error) when calling the PutItem operation (reached max retries: 3): Internal server error: std::runtime_error (cdc::metadata::get_stream: could not find any CDC stream (current time: 2020/07/15 17:26:36). Are we in the middle of a cluster upgrade?)
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
