From 8b2f5979c902ab0bf24fabfb9706db4fb971f542 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Mon, 25 May 2015 19:20:57 -0400 Subject: [PATCH 1/7] sstable tests: replace last then statement with finally Currently, a failed test is leaving files behind. Signed-off-by: Glauber Costa --- tests/urchin/sstable_datafile_test.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/urchin/sstable_datafile_test.cc b/tests/urchin/sstable_datafile_test.cc index 91c69573c1..1a44526bd6 100644 --- a/tests/urchin/sstable_datafile_test.cc +++ b/tests/urchin/sstable_datafile_test.cc @@ -98,7 +98,7 @@ SEASTAR_TEST_CASE(datafile_generation_01) { offset += end_of_row.size(); BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 1); }); }); @@ -169,7 +169,7 @@ SEASTAR_TEST_CASE(datafile_generation_02) { offset += end_of_row.size(); BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 2); }); }); @@ -240,7 +240,7 @@ SEASTAR_TEST_CASE(datafile_generation_03) { offset += end_of_row.size(); BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 3); }); }); @@ -317,7 +317,7 @@ SEASTAR_TEST_CASE(datafile_generation_04) { offset += end_of_row.size(); BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 4); }); }); @@ -384,7 +384,7 @@ SEASTAR_TEST_CASE(datafile_generation_05) { offset += end_of_row.size(); BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 5); }); }); @@ -458,7 +458,7 @@ SEASTAR_TEST_CASE(datafile_generation_06) { offset += end_of_row.size(); BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 6); }); }); @@ -521,7 +521,7 @@ SEASTAR_TEST_CASE(datafile_generation_07) { offset += key2.size(); BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 7); }); }); @@ -598,7 +598,7 @@ SEASTAR_TEST_CASE(datafile_generation_08) { BOOST_REQUIRE(size == offset); }); - }).then([] { + }).finally([] { return remove_files("tests/urchin/sstables", 8); }); }); From 0519dec0d772fddbc378760a53730631e4300d34 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Mon, 25 May 2015 19:38:45 -0400 Subject: [PATCH 2/7] sstable tests: remove filter file It is not being remove, and is thus left as garbage in the sstable directory. Signed-off-by: Glauber Costa --- tests/urchin/sstable_datafile_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/urchin/sstable_datafile_test.cc b/tests/urchin/sstable_datafile_test.cc index 1a44526bd6..b3d172ac43 100644 --- a/tests/urchin/sstable_datafile_test.cc +++ b/tests/urchin/sstable_datafile_test.cc @@ -32,6 +32,7 @@ static inline future<> remove_files(sstring dir, unsigned long generation) { return when_all(remove_file(sstable::filename(dir, la, generation, big, sstable::component_type::Data)), remove_file(sstable::filename(dir, la, generation, big, sstable::component_type::Index)), remove_file(sstable::filename(dir, la, generation, big, sstable::component_type::Summary)), + remove_file(sstable::filename(dir, la, generation, big, sstable::component_type::Filter)), remove_file(sstable::filename(dir, la, generation, big, sstable::component_type::TOC))).then([] (auto t) {}); } From f04f7f2819282ff9b4f42ad2f3079b1b96883613 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Mon, 25 May 2015 14:27:17 -0400 Subject: [PATCH 3/7] sstables: turn composite into a class Encapsulating it into a composite class is a more natural way to handle this, and will allow for extending that class later. Signed-off-by: Glauber Costa --- sstables/key.cc | 2 +- sstables/key.hh | 16 ++++++++++++++-- sstables/sstables.cc | 20 ++++++++++---------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/sstables/key.cc b/sstables/key.cc index d02da5e986..c0b8e873f9 100644 --- a/sstables/key.cc +++ b/sstables/key.cc @@ -119,7 +119,7 @@ key key::from_partition_key(const schema& s, const partition_key& pk) { return from_components(s, pk.begin(s), pk.end(s)); } -bytes composite_from_clustering_key(const schema& s, const clustering_key& ck) { +composite composite::from_clustering_key(const schema& s, const clustering_key& ck) { return from_components(ck.begin(s), ck.end(s), s.clustering_key_type()->types(), true); } diff --git a/sstables/key.hh b/sstables/key.hh index 79c5ce0935..e8e5dd95a3 100644 --- a/sstables/key.hh +++ b/sstables/key.hh @@ -96,8 +96,6 @@ inline key maximum_key() { return key(key::kind::after_all_keys); }; -bytes composite_from_clustering_key(const schema& s, const clustering_key& ck); - class composite_view { bytes_view _bytes; public: @@ -109,4 +107,18 @@ public: return _bytes; } }; + +class composite { + bytes _bytes; +public: + composite (bytes&& b) : _bytes(std::move(b)) {} + static composite from_bytes(bytes b) { return composite(std::move(b)); } + static composite from_clustering_key(const schema& s, const clustering_key& ck); + explicit operator bytes_view() const { + return _bytes; + } + operator composite_view() const { + return composite_view(_bytes); + } +}; } diff --git a/sstables/sstables.cc b/sstables/sstables.cc index 98b51cbc72..917ad2bf97 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -878,7 +878,7 @@ future<> sstable::store() { }); } -static future<> write_composite_component(file_writer& out, disk_string&& column_name, +static future<> write_composite_component(file_writer& out, disk_string_view&& column_name, int8_t end_of_component = 0) { // NOTE: end of component can also be -1 and 1 to represent ranges. return do_with(std::move(column_name), [&out, end_of_component] (auto& column_name) { @@ -888,22 +888,22 @@ static future<> write_composite_component(file_writer& out, disk_string write_column_name(file_writer& out, bytes& clustering_key, const bytes& column_name) { +static future<> write_column_name(file_writer& out, const composite& clustering_key, const bytes& column_name) { // FIXME: This code assumes name is always composite, but it wouldn't if "WITH COMPACT STORAGE" // was defined in the schema, for example. // uint16_t and int8_t are about the 16-bit length and end of component, respectively. - uint16_t size = clustering_key.size() + column_name.size() + sizeof(uint16_t) + sizeof(int8_t); + uint16_t size = bytes_view(clustering_key).size() + column_name.size() + sizeof(uint16_t) + sizeof(int8_t); return write(out, size).then([&out, &clustering_key] { - if (!clustering_key.size()) { + if (!bytes_view(clustering_key).size()) { return make_ready_future<>(); } - return write(out, clustering_key); + return write(out, bytes_view(clustering_key)); }).then([&out, &column_name] { // if size of column_name is zero, then column_name is a row marker. - disk_string c_name; + disk_string_view c_name; c_name.value = column_name; return write_composite_component(out, std::move(c_name)); @@ -913,7 +913,7 @@ static future<> write_column_name(file_writer& out, bytes& clustering_key, const // magic value for identifying a static column. static constexpr uint16_t STATIC_MARKER = 0xffff; -static future<> write_static_column_name(file_writer& out, const bytes& column_name) { +static future<> write_static_column_name(file_writer& out, bytes_view column_name) { // first component of static column name is composed of the 16-bit magic value ff ff, // followed by a null composite, i.e. three null bytes. uint16_t first_component_size = sizeof(uint16_t) + sizeof(uint16_t) + sizeof(int8_t); @@ -926,7 +926,7 @@ static future<> write_static_column_name(file_writer& out, const bytes& column_n return write_composite_component(out, {}); }); }).then([&out, &column_name] { - disk_string c_name; + disk_string_view c_name; c_name.value = column_name; return write_composite_component(out, std::move(c_name)); @@ -970,7 +970,7 @@ static future<> write_cell(file_writer& out, atomic_cell_view cell) { } } -static future<> write_row_marker(file_writer& out, const rows_entry& clustered_row, bytes& clustering_key) { +static future<> write_row_marker(file_writer& out, const rows_entry& clustered_row, const composite& clustering_key) { // Missing created_at (api::missing_timestamp) means no row marker. if (clustered_row.row().created_at() == api::missing_timestamp) { return make_ready_future<>(); @@ -989,7 +989,7 @@ static future<> write_row_marker(file_writer& out, const rows_entry& clustered_r // write_datafile_clustered_row() is about writing a clustered_row to data file according to SSTables format. // clustered_row contains a set of cells sharing the same clustering key. static future<> write_clustered_row(file_writer& out, schema_ptr schema, const rows_entry& clustered_row) { - bytes clustering_key = composite_from_clustering_key(*schema, clustered_row.key()); + auto clustering_key = composite::from_clustering_key(*schema, clustered_row.key()); return do_with(std::move(clustering_key), [&out, schema, &clustered_row] (auto& clustering_key) { return write_row_marker(out, clustered_row, clustering_key).then( From 299a185376c5b07c3a6e81c634262835d28cc7e6 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Mon, 25 May 2015 16:19:04 -0400 Subject: [PATCH 4/7] sstable: create a key from an exploded view Signed-off-by: Glauber Costa --- sstables/key.cc | 4 ++++ sstables/key.hh | 1 + 2 files changed, 5 insertions(+) diff --git a/sstables/key.cc b/sstables/key.cc index c0b8e873f9..3365991c3e 100644 --- a/sstables/key.cc +++ b/sstables/key.cc @@ -123,6 +123,10 @@ composite composite::from_clustering_key(const schema& s, const clustering_key& return from_components(ck.begin(s), ck.end(s), s.clustering_key_type()->types(), true); } +composite composite::from_exploded(const std::vector& v) { + return from_components(v.begin(), v.end(), std::vector(v.size(), bytes_type), true); +} + inline std::vector explode_composite(bytes_view _bytes) { std::vector ret; diff --git a/sstables/key.hh b/sstables/key.hh index e8e5dd95a3..e542f7078f 100644 --- a/sstables/key.hh +++ b/sstables/key.hh @@ -114,6 +114,7 @@ public: composite (bytes&& b) : _bytes(std::move(b)) {} static composite from_bytes(bytes b) { return composite(std::move(b)); } static composite from_clustering_key(const schema& s, const clustering_key& ck); + static composite from_exploded(const std::vector& v); explicit operator bytes_view() const { return _bytes; } From c5b67860947ddd7bbf2beda585156d4e71af011b Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Mon, 25 May 2015 17:39:16 -0400 Subject: [PATCH 5/7] sstables: adjust write_column_name interface The column_name can be comprised of more than one element. This will be the case for collections, that will embed part of the collection data in the column name. While doing this, we also take advantage of the infrastructure we have added to composite. We are duplicating this logic unnecessarily, which is prone to bugs. Those bugs are not just theoretical in this case: we are not writing the final marker for empty composite keys. Signed-off-by: Glauber Costa --- sstables/sstables.cc | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index 917ad2bf97..eafb909a7e 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -888,26 +888,13 @@ static future<> write_composite_component(file_writer& out, disk_string_view write_column_name(file_writer& out, const composite& clustering_key, const bytes& column_name) { +static future<> write_column_name(file_writer& out, const composite& clustering_key, const std::vector& column_names) { // FIXME: This code assumes name is always composite, but it wouldn't if "WITH COMPACT STORAGE" // was defined in the schema, for example. + composite c = composite::from_exploded(column_names); + uint16_t sz = bytes_view(clustering_key).size() + bytes_view(c).size(); - // uint16_t and int8_t are about the 16-bit length and end of component, respectively. - uint16_t size = bytes_view(clustering_key).size() + column_name.size() + sizeof(uint16_t) + sizeof(int8_t); - - return write(out, size).then([&out, &clustering_key] { - if (!bytes_view(clustering_key).size()) { - return make_ready_future<>(); - } - - return write(out, bytes_view(clustering_key)); - }).then([&out, &column_name] { - // if size of column_name is zero, then column_name is a row marker. - disk_string_view c_name; - c_name.value = column_name; - - return write_composite_component(out, std::move(c_name)); - }); + return write(out, sz, bytes_view(clustering_key), bytes_view(c)); } // magic value for identifying a static column. @@ -977,7 +964,7 @@ static future<> write_row_marker(file_writer& out, const rows_entry& clustered_r } // Write row mark cell to the beginning of clustered row. - return write_column_name(out, clustering_key, {}).then([&out, &clustered_row] { + return write_column_name(out, clustering_key, { bytes_view() }).then([&out, &clustered_row] { column_mask mask = column_mask::none; uint64_t timestamp = clustered_row.row().created_at(); uint32_t value_length = 0; @@ -1010,7 +997,7 @@ static future<> write_clustered_row(file_writer& out, schema_ptr schema, const r atomic_cell_view cell = value.second.as_atomic_cell(); const bytes& column_name = column_definition.name(); - return write_column_name(out, clustering_key, column_name).then([&out, cell] { + return write_column_name(out, clustering_key, { bytes_view(column_name) }).then([&out, cell] { return write_cell(out, cell); }); }); From eaa61ba86cf689c5e7f336f6e7781b1c0fbb0aab Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Mon, 25 May 2015 17:48:38 -0400 Subject: [PATCH 6/7] sstables: encode logic for creating a static key component in composite Signed-off-by: Glauber Costa --- sstables/key.cc | 7 +++++++ sstables/key.hh | 1 + 2 files changed, 8 insertions(+) diff --git a/sstables/key.cc b/sstables/key.cc index 3365991c3e..df09607b6e 100644 --- a/sstables/key.cc +++ b/sstables/key.cc @@ -127,6 +127,13 @@ composite composite::from_exploded(const std::vector& v) { return from_components(v.begin(), v.end(), std::vector(v.size(), bytes_type), true); } +composite composite::static_prefix(const schema& s) { + static bytes static_marker(size_t(2), bytes::value_type(0xff)); + + std::vector sv(s.clustering_key_size()); + return static_marker + from_components(sv.begin(), sv.end(), std::vector(sv.size(), bytes_type), true); +} + inline std::vector explode_composite(bytes_view _bytes) { std::vector ret; diff --git a/sstables/key.hh b/sstables/key.hh index e542f7078f..000bda3090 100644 --- a/sstables/key.hh +++ b/sstables/key.hh @@ -115,6 +115,7 @@ public: static composite from_bytes(bytes b) { return composite(std::move(b)); } static composite from_clustering_key(const schema& s, const clustering_key& ck); static composite from_exploded(const std::vector& v); + static composite static_prefix(const schema& s); explicit operator bytes_view() const { return _bytes; } From 87eda7eb5dbdb73960d8762fdfa53fefdf092e15 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Mon, 25 May 2015 17:49:28 -0400 Subject: [PATCH 7/7] sstables: adjust static column name creation interface The column_name can be comprised of more than one element. This will be the case for collections, that will embed part of the collection data in the column name. While doing this, we also take advantage of the infrastructure we have added to composite. We are duplicating this logic unnecessarily, which is prone to bugs. Those bugs are not just theoretical in this case: the static prefix is not "followed by a null composite, i.e. three null bytes." as the code implies. The static prefix is created by appending one empty element for each element in the clustering key, and will only be three null bytes in the case where the clustering key has one element. Signed-off-by: Glauber Costa --- sstables/sstables.cc | 37 ++++++------------------------------- 1 file changed, 6 insertions(+), 31 deletions(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index eafb909a7e..e92befd0f7 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -878,14 +878,6 @@ future<> sstable::store() { }); } -static future<> write_composite_component(file_writer& out, disk_string_view&& column_name, - int8_t end_of_component = 0) { - // NOTE: end of component can also be -1 and 1 to represent ranges. - return do_with(std::move(column_name), [&out, end_of_component] (auto& column_name) { - return write(out, column_name, end_of_component); - }); -} - // @clustering_key: it's expected that clustering key is already in its composite form. // NOTE: empty clustering key means that there is no clustering key. static future<> write_column_name(file_writer& out, const composite& clustering_key, const std::vector& column_names) { @@ -897,27 +889,12 @@ static future<> write_column_name(file_writer& out, const composite& clustering_ return write(out, sz, bytes_view(clustering_key), bytes_view(c)); } -// magic value for identifying a static column. -static constexpr uint16_t STATIC_MARKER = 0xffff; +static future<> write_static_column_name(file_writer& out, const schema& schema, const std::vector& column_names) { + composite sp = composite::static_prefix(schema); + composite c = composite::from_exploded(column_names); + uint16_t sz = bytes_view(sp).size() + bytes_view(c).size(); -static future<> write_static_column_name(file_writer& out, bytes_view column_name) { - // first component of static column name is composed of the 16-bit magic value ff ff, - // followed by a null composite, i.e. three null bytes. - uint16_t first_component_size = sizeof(uint16_t) + sizeof(uint16_t) + sizeof(int8_t); - uint16_t second_component_size = column_name.size() + sizeof(uint16_t) + sizeof(int8_t); - uint16_t total_size = first_component_size + second_component_size; - - return write(out, total_size).then([&out] { - - return write(out, STATIC_MARKER).then([&out] { - return write_composite_component(out, {}); - }); - }).then([&out, &column_name] { - disk_string_view c_name; - c_name.value = column_name; - - return write_composite_component(out, std::move(c_name)); - }); + return write(out, sz, bytes_view(sp), bytes_view(c)); } // Intended to write all cell components that follow column name. @@ -1014,9 +991,7 @@ static future<> write_static_row(file_writer& out, schema_ptr schema, const row& } assert(column_definition.is_static()); atomic_cell_view cell = value.second.as_atomic_cell(); - const bytes& column_name = column_definition.name(); - - return write_static_column_name(out, column_name).then([&out, cell] { + return write_static_column_name(out, *schema, { bytes_view(column_definition.name()) }).then([&out, cell] { return write_cell(out, cell); }); });