With this we can construct a data_value from any string type. This
also avoids a few sstring copies.
Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
This avoids a copy in the callers. While at it, also make this
function non-virtual since it is never overwritten.
Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
This moves the string copy from the callers to the implementation of
to_internal_name.
Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
"
Reverse queries work by reading an entire partition into memory, then
start emitting its rows in reverse order. It is easy to see how this can
lead to disasters combined with large partitions. In fact a handful of
such reverse queries on large partitions is enough to bring a node down.
To prevent this, abort reverse queries, when we find out that the size
of the partition is larger than a limit. This might be annoying to users,
but I'm sure it is not as annoying as their nodes going down.
The limit is configurable via `max_memory_for_unlimited_query`
configuration option, which is 1MB by default. This limit is propagated
to each table, system tables having no limit. This limit is planned to
be used by other queries capable of consuming unlimited amount of
memory, like unpaged queries. Not in this series.
The proper solution would be to read the data in reverse (#1413), but
that is a major effort. In the meanwhile make sure the unsuspecting user
won't bring their nodes down with an innocent looking ordering
directive.
Note that for calculating the memory footprint of the
partition-in-question, only the clustering rows are used. This should be
fine, the 1MB limit is conservative enough that an eventual overshoot
caused by the omitted range tombstones and the static row would not make
a big difference.
Fixes: #5804
"
* 'limit-reverse-query-memory-consumption/v3' of https://github.com/denesb/scylla:
flat_mutation_reader: make_reversing_reader(): add memory limit
db/config: add config memory limit of otherwise unlimited queries
utils::updateable_value: add operator=(T)
flat_mutation_reader: expose reverse reader as a standalone reader
A static constructor was used to initialize update_row_query. That
constructor would call meta::roles_table::qualified_name() which would
access AUTH_KS which is also initialized by a static constructor in
another file, so the construction order is not guaranteed.
This change turns update_row_query into a function with a static local
variable in it. The static local is initialized at first use, fixing
the problem.
Signed-off-by: Rafael Ávila de Espíndola <espindola@scylladb.com>
Message-Id: <20200227163916.19761-1-espindola@scylladb.com>
Merged patch series by Piotr Sarna:
This series makes json parsing yieldable in order
to prevent reactor stalls. It's done by:
1. Extracting the parsing stage out of alternator executor
2. Moving the parsing stage to a separate service,
which uses a static seastar thread (parallelism: 1)
3. Wrapping rjson parsing routines with a yieldable parser,
which takes advantage of running in a seastar thread
and occasionally performs maybe_yield()
Step 2 above is only used for JSON's big enough to potentially
create stalls - small requests will be parsed immediately,
without being redirected to a static thread.
Handling a PutItem operation with large JSONs
on my machine takes approximately:
1MB doc: ~30ms
3MB doc: ~90ms
12MB doc: ~350ms
out of which parsing itself is around:
1MB doc: ~7ms
3MB doc: ~20ms
12MB doc: ~80ms
(bonus: 400KiB doc: ~2ms)
; the document was a single object full of small items,
which triggers many allocations during parsing.
The above numbers were roughly the same before and after
the series, but the 12MB document did not cause reactor
stalls after the patch.
Note: writing the JSON can still be a source of stalls,
especially for large documents.
Note2: DynamoDB limits single value size to 400KiB,
but for batches it will be 16MiB total request size
Note3: If parallelism ever proves to be an issue,
it's easily increasable by spawning more static threads.
Refs: #5742
Tests: alternator(local)
manual
Piotr Sarna (12):
alternator: break lines in server callbacks
alternator: allow moving the request from rmw operation
alternator: move parsing in front of executor
alternator: convert parse to std::string_view
alternator: implement json parser inside the server
alternator: remove rjson::parse_raw
alternator: make rjson yieldable in thread context
alternator: fix returning raw JSON errors
alternator: change json errors class to SerializationException
alternator-test: rename large requests test to 'manual requests'
alternator-test: extract getting signed request helper
alternator-test: add tests for incorrect JSON documents
...ge_requests.py => test_manual_requests.py} | 53 +++--
alternator/executor.cc | 203 ++++++++----------
alternator/executor.hh | 33 +--
alternator/rjson.cc | 47 +++-
alternator/rjson.hh | 7 +-
alternator/rmw_operation.hh | 1 +
alternator/serialization.cc | 9 +-
alternator/server.cc | 111 ++++++++--
alternator/server.hh | 20 +-
9 files changed, 310 insertions(+), 174 deletions(-)
rename alternator-test/{test_large_requests.py => test_manual_requests.py} (70%)
When has_relevant_range_on_this_shard() found a relevant range, it will unnecessarily
iterate through the end. Verified manually that this could be thousands of pointless
iterations when streaming data to a node just added. The relevant code could be
simplified by de-futurizing it but I think it remains so to allow task scheduler
to preempt it if necessary.
Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>
Message-Id: <20200220224048.28804-2-raphaelsc@scylladb.com>
This test suite can then be the parent of tests which use custom,
potentially not validated input in order to test alternator
against data not easy to push via boto3 or Python, due to their
implementation details.
Previously, alternator server was not directly sharded - and instead
kept a helper http server control class, which stored sharded http
server inside. That design is confusing and makes it hard to expand
alternator server with new sharded attributes, so from now on
the alternator server is itself sharded<>.
Tests: alternator-test(local, smp==1&smp==4)
Fixes#5913
Message-Id: <b50e0e29610c0dfea61f3a1571f8ca3640356782.1582788575.git.sarna@scylladb.com>
In order to be consistent with DynamoDB - a parsing error on incorrect
JSON input is reported as SerializationException instead of
ValidationException.
A couple of places in executor code leaked raw JSON errors to the user
instead of formulating a proper ValidationException message.
These places are now fixed, and the next patch in this series will
act as a regression checker, since all JSON errors will be returned
as SerializationException, not ValidationException instances.
In order to fight reactor stalls, rjson parsing and writing
routines can now yield if they run in seastar thread context.
In order to run a yieldable version of the parser which needs
to be run in seastar thread context, use parse_yieldable()
instead of parse().
The json parser runs in a static thread which accepts and parses
documents. Documents smaller than a parsing threshold
(currently: 16KiB) will be parsed in place without yielding.
The assumption is that most alternator requests are small
and there's no need to parse them in a yieldable way,
which also induces overhead. For reference, parsing a 128KiB
document made of many small objects with rapidjson takes
around 0.5 millisecond, and a 16KiB document is parsed
in around 0.06ms - a value small enough not to disturb
Seastar's current value of 0.5ms task quota too much.
Parsing a request string into JSON happens as a first thing
in every request, so it can be performed before calling
any executor callbacks. The most important thing however,
is that making parsing a separate stage allows certain optimizations,
e.g. running all parsing in a single seastar thread, which allows
adding yields to rjson parsing later.
If the reversing requires more memory than the limit, the read is
aborted. All users are updated to get a meaningful limit, from the
respective table object, with the exception of tests of course.
We have a few kind of queries whose memory consumption is not limited at
all. One of these is reverse queries, which reads entire partitions into
memory, before reversing them. These partitions can be larger than
memory and thus such a query can single-handedly cause OOM.
This patch introduces a configuration for a memory limit for such
queries. This will serve as a hard limit and queries which attempt to
use more memory than this, will be aborted.
The limit is propagated to table objects, with the intention of keeping
system tables unlimited. These tables are usually small and initiators
of system queries are not prepared for failures.
Currently reverse reads just pass a flag to
`flat_mutation_reader::consume()` to make the read happen in reverse.
This is deceptively simple and streamlined -- while in fact behind the
scenes a reversing reader is created to wrap the reader in question to
reverse partitions, one-by-one.
This patch makes this apparent by exposing the reversing reader via
`make_reversing_reader()`. This now makes how reversing works more
apparent. It also allows for more configuration to be passed to the
reversing reader (in the next patches).
This change is forward compatible, as in time we plan to add reversing
support to the sstable layer, in which case the reversing reader will
go.
The CMake build system in seastar.git exports the package to CMake
package registry. However, we don't use it when building from scylla.git
(we link to seastar directly) and get the following warning when
building with "dbuild" (that does not bind mount $HOME/.cmake):
CMake Warning at CMakeLists.txt:1180 (export):
Cannot create package registry file:
/home/penberg/.cmake/packages/Seastar/3b6ede62290636bbf1ab4f0e4e6a9e0b
No such file or directory
Let's just disable the package registry for our builds by setting the
CMAKE_EXPORT_NO_PACKAGE_REGISTRY CMake option as discussed here to make
the warning go away:
https://cmake.org/cmake/help/v3.4/variable/CMAKE_EXPORT_NO_PACKAGE_REGISTRY.html
Message-Id: <20200227092743.27320-1-penberg@scylladb.com>
To install scylla using install.sh easily, we need to run following things:
- add scylla user/group
- configure scylla.yaml
- run scylla_post_install.sh
But we don't want to run them when we build .rpm/.deb package,
we also need to add --packaging option to skip them.
Fixes#5830
"
Here is a simple introduction to the node operations scylla supports and
some of the issues.
- Replace operation
It is used to replace a dead node. The token ring does not change. It
pulls data from only one of the replicas which might not be the
latest copy.
- Rebuild operation
It is used to get all the data this node owns form other nodes. It
pulls data from only one of the replicas which might not be the
latest copy.
- Bootstrap operation
It is used to add a new node into the cluster. The token ring
changes. Do no suffer from the "not the latest replica” issue. New
node pulls data from existing nodes that are losing the token range.
Suffer from failed streaming. We split the ranges in 10 groups and we
stream one group at a time. Restream the group if failed, causing
unnecessary data transmission on wire.
Bootstrap is not resumable. Failure after 99.99% of data is streamed.
If we restart the node again, we need to stream all the data again
even if the node already has 99.99% of the data.
- Decommission operation
It is used to remove a live node form the cluster. Token ring
changes. Do not suffer “not the latest replica” issue. The leaving
node pushes data to existing nodes.
It suffers from resumable issue like bootstrap operation.
- Removenode operation
It is used to remove a dead node out of the cluster. Existing nodes
pulls data from other existing nodes for the new ranges it own. It
pulls from one of the replicas which might not be the latest copy.
To solve all the issues above. We could use repair based node operation.
The idea behind repair based node operations is simple: use repair to
sync data between replicas instead of streaming.
The benefits:
- Latest copy is guaranteed
- Resumable in nature
- No extra data is streamed on wire
E.g., rebuild twice, will not stream the same data twice
- Unified code path for all the node operations
- Free repair operation during bootstrap, replace operation and so on.
Fixes: #3003Fixes: #4208
Tests: update_cluster_layout_tests.py + replace_address_test.py + manual test
"
* 'repair_for_node_ops' of https://github.com/asias/scylla:
docs: Add doc for repair_based_node_ops
storage_service: Enable node repair based ops for bootstrap
storage_service: Enable node repair based ops for decommission
storage_service: Enable node repair based ops for replace
storage_service: Enable node repair based ops for removenode
storage_service: Enable node repair based ops for rebuild
storage_service: Use the same tokens as previous bootstrap
storage_service: Add is_repair_based_node_ops_enabled helper
config: Add enable_repair_based_node_ops
repair: Add replace_with_repair
repair: Add rebuild_with_repair
repair: Add do_rebuild_replace_with_repair
repair: Add removenode_with_repair
repair: Add decommission_with_repair
repair: Add do_decommission_removenode_with_repair
repair: Add bootstrap_with_repair
repair: Introduce sync_data_using_repair
repair: Propagate exception in tracker::run
* seastar 7a3b4b4e4e...affc3a5107 (6):
> Merge "Add the possibility to remove rules from routes" from Pavel
> stall_detector: expose correct clock type to use
> queue: add has_blocked_consumer() function
> Merge "core: reduce memory use for idle connections" from Avi
> testing: Enable abort_on_internal_error on tests
> core: Add a on_internal_error helper
When we test Alternator on its HTTPS port (i.e., pytest --https),
we don't want requests to verify the pedigree of the SSL certificate.
Our "dynamodb" fixture (conftest.py) takes care of this for most of
the tests, but a few tests create their own requests and need to pass the
"verify=False" option on their own. In some tests, we forgot to do
this, and this patch fixes three tests which failed with "pytest --https".
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200226142330.27846-1-nyh@scylladb.com>
Merged pull request https://github.com/scylladb/scylla/pull/5897
from Juliusz Stasiewicz:
Column operation now contains operation::row_delete (== 2)
after queries like delete from tbl where pk=x and ck=y;. Before
this patch row deletes were treated as updates, which was incorrect
because updates do not contain row tombstones (and row deletes do).
Refs #5709
Column `operation` now contains `operation::row_delete` (== 2)
after queries like `delete from tbl where pk=x AND ck=y;`. Before
this patch row deletes were treated as updates, which was incorrect
because updates do not contain row tombstones (and row deletes do).
Refs #5709
Merged patch series from Piotr Sarna:
Alternator shutdown routines were only registered in main.cc,
but it's not enough - other operations, like decommision,
also rely on shutting down client servers.
In order to remedy the situation, a notion of client shutdown
listeners is introduced to storage service.
A shutdown listener implements a callback used by the storage
service when client servers need to shut down, and at the same
time it does not force storage service to keep a reference
for the client service itself.
NOTE: the interface can also be used later to provide
proper shutdown routines for redis and any other future APIs.
Fixes#5886
Tests: alternator-test(local, including a shutdown during the run)
Piotr Sarna (4):
storage_service: make shutdown_client_servers() thread-only
storage_service: add client shutdown hook
main: make alternator shutdown hook-based
main: reduce scope of alternator services
main.cc | 18 +++++++++---------
service/storage_service.cc | 22 +++++++++++++++++-----
service/storage_service.hh | 15 ++++++++++++++-
3 files changed, 40 insertions(+), 15 deletions(-)
On some environment systemd-coredump does not work with symlink directory,
we can use bind-mount instead.
Also, it's better to check systemd-coredump is working by generating coredump.
Fixes#5753
With the new shutdown routines in place, alternator executor
and server do not need to be declared outside of the `if` clause
which conditionally sets up alternator.
