Ok, shame on me: the version string was so obviously correct that I only
verified that the comparisons were working as expected.
Turns out it isn't: http://lists.boost.org/boost-users/2006/12/24194.php
boost::format will treat uint8_t arguments as char, and therefore we will end
up with the version string misprinted.
We can just cast it to uint16_t before we print, but since this is not exactly
a struct that we will be using all the time, let's favor readability over
saving a few bytes, and change all fields to uint16_t.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Some code may attempt to use it during finalization after "instance"
was destroyed.
Reported by Pekka:
/usr/include/c++/4.9.2/bits/unique_ptr.h:291:14: runtime error:
reference binding to null pointer of type 'struct
standard_allocation_strategy'
./utils/allocation_strategy.hh:105:13: runtime error: reference
binding to null pointer of type 'struct standard_allocation_strategy'
./utils/allocation_strategy.hh:118:35: runtime error: reference
binding to null pointer of type 'struct allocation_strategy'
./utils/managed_bytes.hh:59:45: runtime error: member call on null
pointer of type 'struct allocation_strategy'
./utils/allocation_strategy.hh:82:9: runtime error: member access
within null pointer of type 'struct allocation_strategy'
As Avi suggested, we can use a tuple to make some comparisons more natural.
However, instead of doing a make_tuple on the comparison only, we can go
further and store the tuple internally.
I am still keeping the outer type, so it can host convenience functions like
to_sstring() and current().
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
"With the present patchset, we are now able to
- start cassandra-2.1.8
- generate a keyspace and a table
- flush it
- exit it
- start scylla on that same directory
And it works, from the sstable point of view. Note that if we don't flush,
Cassandra-2.1.8 won't do that automatically (it seems to do for 2.2), and
things will be left in the commitlog. We are not replaying their commitlog,
so we won't see any data.
The reverse also works - starting scylla, creating a table, exiting it.
After starting cassandra-2.1.8 in the same directory, everything works fine.
There are still some minor issues left, but they are not showstoppers. I will
open individual issues for each of them."
There is another field I missed, index_interval. It is not actually used for
2.1.8 - so that's why it is easy to stop, but it at least exists.
2.1.8 already has "min_index_interval" and "max_index_interval". If we see a
table that contains index_interval, that will become "min_index_interval".
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
They are multi-cell in Origin. This has nothing to do with 2.2 vs 2.1,
and it is just a plain bug.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
This is the biggest change from 2.2: for the 2.1 series, the default type is
always stored in the comparator for compound types.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
The class I am presenting will make it easier for us to compare it with desired
versions so we can control proper behavior when needed.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
They should be set. As a result, those columns will have the index "null"
at the schema_columns table.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
We are currently assigning non-partition keys the index 0. That is not what
happens in Origin:
cqlsh> create table ks.twoclust \
(ks int, cl1 int, cl2 int, r1 text, r2 text, primary key (ks, cl1, cl2));
cqlsh> select columnfamily_name, column_name, component_index \
from system.schema_columns where keyspace_name='ks';
columnfamily_name | column_name | component_index
-------------------+-------------+-----------------
twoclust | cl1 | 0
twoclust | cl2 | 1
twoclust | ks | null
twoclust | r1 | 2
twoclust | r2 | 2
This is happening because we use column.position(), which has no knowledge of
the clustering keys at all. We should instead pass that by the schema, which
will then do the right thing.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
We will invoke the schema builder from schema_tables.cc, and at that point, the
information about compact storage no longer exists anywhere. If we just call it
like this, it will be the same as calling it with compact_storage::no, which
will trigger a (wrong) recomputation for compact_storage::yes CFs
The best way to solve that, is make the compact_storage parameter mandatory
every time we create a new table - instead of defaulting to no. This will
ensure that the correct dense and compound calculation are always done when
calling the builder with a parameter, and not done at all when we call it
without a parameter.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
If we alter the compound property, we also have to rebuild the schema,
since some aspects of the columns depend on it. Let's just go ahead and
always rebuild the schema.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
We will use those properties during initialization - for instance, to calculate
thrift_bits.is_on_all_components. In order to do that, it has to be available at
schema creation, and not through the schema builder.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
This is basically a glorified map<sstring, sstring>, that does some validation
on the options. Analoguous structures in the past were put directly at
schema.hh, but I will keep this one separate because it got slightly more
complex than the average.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
"This still doesn't achieve compatibility with Cassandra-2.1.8, my next series
will. But let's merge this one first so the rest can be reviewed separately."
Without this, Cassandra won't even try to read our sstables. The containing
directories will be ignored.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
This table exists in 2.1.8, and although it is dropped in 2.2, we
should at least list its schema.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
2.1.8 tables have 3 more fields in their system tables, that 2.2 don't.
Since we aim at 2.1 compatibility, we have to include them.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
They do not exist in 2.2, and don't serve a huge purpose. But we will
need them for compatibility with 2.1
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Let's leave their schema in here, since it's ready and we may need them in the
future. But since they are not present in 2.1.8, we will remove them from the
schema list.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Don't let the current name fool you: Having this listed as "la" here
was just lack of discipline on my part. I meant by it "the format from
which we are importing" - which was named la for Origin. I wasn't
really thinking at the time that it would be dangerous to stop between
versions.
This should read ka, not la.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Let's change the default generated tables to ka, which is the one that is present
in Origin
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
A ka file has a slightly different name on disk. Change the
parser so we can deal with both
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
When a schema is available, we use it. However, we have, by now, way too many
tests. Some of them use tables for which we don't even know the schema. It would
have been a massive amount of work to require a schema for all of them - so I am
keeping both constructors around.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
We have currently two versions of filename: one static, where the caller has to
pass all parameters, and an internal one where those parameters are derived
from the sstable attributes. Implement the latter in terms of the former so
making changes gets easier.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
It is currently only used to log a message, and for that we have an sstable
method that will do just fine. Using the name itself just makes it being passed
along throughout the captures. Remove it.
Signed-off-by: Glauber Costa <glommer@cloudius-systems.com>
Add a test case that triggers the heap overflow fixed in previous commit
("bytes_ostream: Fix current_space_left()").
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
We also allocate chunks larger than "usable_chunk_size" in alloc(). Fix
up the calculation in current_space_left().
Spotted by ASan.
Signed-off-by: Pekka Enberg <penberg@cloudius-systems.com>
If the requested range wraps around the end of the ring, make_local_reader()
ends up trying to create an sstable reader with that wrapping range, which
is not supported. Also our shard-looping code is wrong in a wrapping range.
The solution is simple: split the wrap-around range into two (from the start
of the range until the end of the ring, and then from the beginning of the
ring until the end of the range), and read each of these subranges normally.
This feature is needed to allow streaming a range that wraps around, because
streaming currently uses make_local_reader().
Signed-off-by: Nadav Har'El <nyh@cloudius-systems.com>
make_local_reader takes a partition-range parameter, passed by *reference*.
The meaning of this passing-by-reference is not documented: when
make_local_reader returns, which of the following two statements holds?
1. make_local_reader still holds this reference, so the caller is
forced to keep the range object alive. For how long?
or
2. make_local_reader reads the range before returning, and when it
returns, the caller continues to "own" the range object, and is
allowed to do anything, including delete it.
The principle of least surprise suggests that #2 is better, unless
there is a very good reason to choose #1, and unless this requirement to
keep the argument alive (and for how long) was clearly documented.
But neither is the case here - the overhead of copying the argument is
negligable compared to the other overheads of make_local_reader, and
nothing was documented.
But it turns out the current code did #1 - the address of the range was
passed around *after* make_local_reader returns - the different readers
on different cpus continue to use it to eventually find the right sstable
byte range to iterate over. It very easy to forget this and call
make_local_reader on a on-stack range variable, and the result is a
hard-to-debug use-after-free mess.
This patch switches us to situation #2: Before make_local_reader returns,
the range is copied to whoever needs to hold it after the return, namely
each individual shard_reader. The patch to do this is trivial (one
character removed).
Signed-off-by: Nadav Har'El <nyh@cloudius-systems.com>
