Up until now, we were still generating one future per element that we write.
Now that we have new infrastructure, we can avoid that, and generate only the
ones we really need to. This has the added advantage of lifting the need to do
lambda captures and allowing for a more straightfoward forwarding of rest...
parameters
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
We always return a future, but with the threaded writer, we can get rid of
that. So while reads will still return a future, the writer will be able to
return void.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
For some reason, I added a fsync call when the file underlying the
stream gets truncated. That happens when flushing a file, which
size isn't aligned to the requested DMA buffer.
Instead, fsync should only be called when closing the stream, so this
patch changes the code to do that.
Signed-off-by: Raphael S. Carvalho <raphaelsc@cloudius-systems.com>
Glauber says:
"The current series fixes a small conversion bug and brings some much needed
cleanups (like in key.cc).
With that in place, it implements and wires support for collections and range
tombstones."
We're looking up shared_ptr<column_identifier> type so make sure we
lookup by value, not by pointer.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
We did so because passing shared pointers in the old code was so much easier.
But it is no longer, so we can avoid the reference bump.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
This is the code to write a range tombstone. This is not yet wired up to actually do it.
The use case for collections is a lot simpler, and will be handled first.
The actual code should be virtually identical, though.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
We can insert markers in the end of composites, which can be used to identify
the presence of ranges in a column.
One option, would be to change all methods in sstables/key.hh to take an
optional marker parameter, and append that as the last marker.
But because we are talking about a single byte, and always added to the end,
it's a lot easier to allow the composite to be created normally, and then
replace the last byte with the marker.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Tomek got confused with the fact that we had to pass bytes_type for this code
to work. And well, that's understandable: that code evolved quite a bit since
its first user, and now the interface is not quite the best for the job,
forcing us to employ weird tricks like that for the code to work.
In this cleanup, I am creating a serializer object, that will encode
information about how to serialize the component passed. In the majority of the
cases, a simple sstable_serializer will just serialize to itself - accepting as
parameters byte_views.
In the case we need to operate on a deeply exploded view - the only case for
which we truly needed types, the respective serializer will take a types vector
and use it accordingly.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
In a testament to how confusing our old code was, while collapsing the futures
I ended up getting the end_of_row element inside the loop for clustered keys.
The end of row, as the name implies, should only be written at the end of the
(thrift) row (= CQL partition).
Move it to the right place.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
writer.hh includes sstables.hh which includes writer.hh
We can't remove the reference if we include core/fstream.hh into writer.hh instead
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
From Glauber:
"This my attempt to convert the sstable write code to seastar threads.
The code does look a lot cleaner now, and the future path on how to improve
it, a bit more clear."
This code is blatantly wrong, because it writes stack variables to the
underlying storage.
After this patch, the code is no longer wrong. Right is better than wrong,
so we should apply this.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Technically speaking, the current code is not wrong. However, it was written
before we had do_with, and I ended up dowithing it while chasing our erratic
bug under the suspicion that this code could somehow be related with our bug.
Turns out it isn't, but now that I went through the trouble of dowithing it -
and since do_with is easier to reason about and guarantee liveness, let's go
with this option.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
checksum and crc are written inside the main function so we don't need
to export the file stream. But since the functions are actually trivial
we can just .get() the whole thing instead of changing them.
The others are still kept as futures and called after async::thread completes,
for maximum parallelism.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
After the last round of cleanups, this function turned out to be exactly the
same as write_column_name, except that the composite differs. Because the
composite is passed as a parameter, we can just use the same function for all
and pre-create the composite.
This will make the implementation of collections a lot easier, since for
collections we will prepend each element with the column name.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
This is my attempt to convert the sstable code to seastar threads, as
we have been extensively discussing. I haven't yet measured how much do we
gain by this, but the final code looks *so* much better and less complicated,
that this alone should be enough reasoning.
Here's how I've done it, so you can easily follow:
Every function that we use and returns a future, is copied to another function
with the same name but ending in _t. This is better than copying the whole thing,
because it can be done in logical pieces that are easier to follow. This is also
easier to verify.
function_t() will do the same as function(), but will return void.
I am not changing more than I need to, so in the final code, without all the
do_withs and other stuff, there are some parts that start to cry for a cleanup.
They are left as is for now, and I will return to them later once the patch is
merged.
In this initial patch, you get the main write_components converted, and this
nice explanation message.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
This code is blatantly wrong, because it writes stack variables to the
underlying storage.
After this patch, the code is no longer wrong. Right is better than wrong,
so we should apply this.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Reviewed-by: Pekka Enberg <penberg@cloudius-systems.com>
Technically speaking, the current code is not wrong. However, it was written
before we had do_with, and I ended up dowithing it while chasing our erratic
bug under the suspicion that this code could somehow be related with our bug.
Turns out it isn't, but now that I went through the trouble of dowithing it -
and since do_with is easier to reason about and guarantee liveness, let's go
with this option.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Reviewed-by: Pekka Enberg <penberg@cloudius-systems.com>
When a promise that still tracks its future is destroyed, but after
future::get() or future::then() was called, the promise thinks it was
destroyed before generating a value, and fails an assertion.
Fix be detaching the promise from the future as soon as get() or then()
remove the value.
Make 'create table' statements also specify the following for
schema_ptrs:
- Partition keys
- Regular columns
- Static columns
Please note that clustering keys are _not_ included because we seem to
lack infrastructure like CompoundType and CellNameType to properly
enable them.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
Switch to using schema_builder in create_table_statement in preparation
for also defining columns in the resulting schema_ptr.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
Move create_table_statement code out-of-line in preparation for
modifying the implementation.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
cql_query_test segfaults in debug mode due to infinite recursion (see
trace below). The problem started to appear after Avi's always-defer
change.
It looks like the following boost::any() constructor template is
instantiated when boost::any is copied into a lambda:
// Perfect forwarding of ValueType
template<typename ValueType>
any(ValueType&& value, typename boost::disable_if<boost::is_same<any&, ValueType> >::type* = 0)
: content(new holder< typename remove_reference<ValueType>::type >(static_cast<ValueType&&>(value)))
{
}
It checks for any& to disable itself on forwarding, but it doesn't
check for const any&. There are (non-template) constructors which take
const any&, but I guess some C++ rule favors the template
version. This results in infinite recursion of constructor invocations
between any() and holder().
Workaround is to make the lambda mutable, so that the argument type is
any& and template doesn't kick in.
Trace:
CU 0x4debed5, DIE 0x4f176e6>, this=0x602000044550) at /usr/include/boost/any.hpp:175
ql_query_test, CU 0x4debed5, DIE 0x4f6c509>, this=0x602000044518) at /usr/include/boost/any.hpp:72
CU 0x4debed5, DIE 0x4f176e6>, this=0x602000044510) at /usr/include/boost/any.hpp:175
ql_query_test, CU 0x4debed5, DIE 0x4f6c509>, this=0x6020000444e8) at /usr/include/boost/any.hpp:72
CU 0x4debed5, DIE 0x4f176e6>, this=0x6020000444e0) at /usr/include/boost/any.hpp:175
oost::any const>, void>::type*) (this=0x7ffff2ffa6f0, value=<unknown type in /home/tgrabiec/src/urchin2/build
/release/tests/urchin/cql_query_test, CU 0x4debed5, DIE 0x4f6c509>) at /usr/include/boost/any.hpp:72
5u> const&, std::vector<boost::any, std::allocator<boost::any> >, std::vector<boost::any, std::allocator<boos
t::any> >, basic_sstring<char, unsigned int, 15u> const&, boost::any)::{lambda(database&)#1}::operator()(data
base&) const::{lambda(std::unique_ptr<mutation_partition const, std::default_delete<mutation_partition> >)#1}
::unique_ptr(std::unique_ptr<mutation_partition const, std::default_delete<mutation_partition> >&&) (this=0x7
ffff2ffa6c0) at tests/urchin/cql_test_env.cc:126
We want query_local() to actually respect the key we pass to it. Fixes
an issue in keyspace merging code where we returned multiple rows for a
keyspace.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
From Pekka:
"This series fixes two issues in schema merging code that caused
cql_query_test to sometimes think that two identical keyspaces are
different. This forced the code to use the alter keyspace paths which
are not implemented and just throw an exception."
We must compare types with ->equal(). Fixes keyspace merging issues
where the merging code mistakenly thinks two keyspaces are different and
calls the alter keyspace path which is not implemented.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
Fix the check in read_schema_for_keyspaces() to not insert empty
result sets in the return value. There's no functional change as the
merge algorithms already deal with the case. However, the code is now
closer to what origin does.
Spotted while reading the code.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
This reverts commit a19d2171eb.
This commit breaks cql_query_test.
[asias@hjpc urchin]$ ./cql_query_test
Running 1 test case...
WARNING: Not implemented: COMPACT_TABLES
WARNING: Not implemented: METRICS
WARNING: Not implemented: PERMISSIONS
cql_query_test: core/distributed.hh:290: Service&
distributed<Service>::local() [with Service =
service::storage_service]: Assertion `local_is_initialized()' failed.
unknown location(0): fatal error in "test_create_keyspace_statement":
signal: SIGABRT (application abort requested)
tests/test-utils.cc(31): last checkpoint
*** 1 failure detected in test suite "tests/urchin/cql_query_test.cc"
(gdb) bt
#0 0x00000032930348d7 in __GI_raise (sig=sig@entry=6) at
../sysdeps/unix/sysv/linux/raise.c:55
#1 0x000000329303653a in __GI_abort () at abort.c:89
#2 0x000000329302d47d in __assert_fail_base (fmt=0x3293186cb8
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=assertion@entry=0x8ec10a "local_is_initialized()",
file=file@entry=0x92508d "core/distributed.hh",
line=line@entry=290, function=function@entry=0x8ed440
<distributed<service::storage_service>::local()::__PRETTY_FUNCTION__>
"Service& distributed<Service>::local() [with Service =
service::storage_service]")
at assert.c:92
#3 0x000000329302d532 in __GI___assert_fail (assertion=0x8ec10a
"local_is_initialized()", file=0x92508d "core/distributed.hh",
line=290,
function=0x8ed440
<distributed<service::storage_service>::local()::__PRETTY_FUNCTION__>
"Service& distributed<Service>::local() [with Service =
service::storage_service]") at assert.c:101
#4 0x0000000000430f19 in local (this=<optimized out>) at
core/distributed.hh:290
#5 get_local_storage_service () at service/storage_service.hh:3326
#6 keyspace::create_replication_strategy (this=0x7ffff6bf8350) at
database.cc:690
#7 0x000000000061537a in
_ZZZN2db20legacy_schema_tables15merge_keyspacesERN7service13storage_proxyEOSt3mapI13basic_sstringIcjLj15EE13lw_shared_ptrIN5query10result_setEESt4lessIS6_ESaISt4pairIKS6_SA_EEESI_ENKUlRT_E0_clISt6ve
ctorISF_SG_EEEDaSK_ENKUlR8databaseE_clESQ_ () at
db/legacy_schema_tables.cc:584
#8 0x0000000000617d19 in operator() (__closure=0x7ffff6bf8650) at
./core/distributed.hh:284
In the test, storage_service and other services are not stared.
Let's revert it and figure out a way to run cql_query_test with the
needed services started properly and then bring the "storage_service:
Remove ad-hoc token_metadata creation" change back.
