From 031bf57c1948e9651e358d6ba11cc6ca5b562be6 Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Thu, 10 Mar 2016 18:00:27 -0300 Subject: [PATCH] sstables: bail out if toc exists for generation used by write_components Currently, if sstable::write_components() is called to write a new sstable using the same generation of a sstable that exists, a temporary TOC will be unconditionally created. Afterwards, the same sstable::write_components() will fail when it reaches sstable::create_data(). The reason is obvious because data component exists for that generation (in this scenario). After that, user will not be able to boot scylla anymore because there is a generation with both a TOC and a temporary TOC. We cannot simply remove a generation with TOC and temporary TOC because user data will be lost (again, in this scenario). After all, the temporary TOC was only created because sstable::write_components() was wrongly called with the generation of a sstable that exists. Solution proposed by this patch is to trigger exception if a TOC file exists for the generation used. Some SSTable unit tests were also changed to guarantee that we don't try to overwrite components of an existing sstable. Refs #1014. Signed-off-by: Raphael S. Carvalho Message-Id: --- sstables/sstables.cc | 5 +++++ tests/sstable_test.cc | 40 +++++++++++++++++++++++----------------- tests/sstable_test.hh | 4 ++++ 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index 6aaf432ca7..17c45111c2 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -755,6 +755,11 @@ void sstable::write_toc(const io_priority_class& pc) { sstlog.debug("Writing TOC file {} ", file_path); + bool toc_exists = file_exists(filename(sstable::component_type::TOC)).get0(); + if (toc_exists) { + throw std::runtime_error(sprint("SSTable write failed due to existence of TOC file for generation %ld of %s.%s", _generation, _ks, _cf)); + } + // Writing TOC content to temporary file. file f = new_sstable_component_file(file_path, open_flags::wo | open_flags::create | open_flags::truncate).get0(); diff --git a/tests/sstable_test.cc b/tests/sstable_test.cc index 9930aba9df..4260223425 100644 --- a/tests/sstable_test.cc +++ b/tests/sstable_test.cc @@ -36,6 +36,7 @@ #include "database.hh" #include #include "sstable_test.hh" +#include "tmpdir.hh" using namespace sstables; @@ -169,10 +170,11 @@ SEASTAR_TEST_CASE(big_summary_query_32) { return summary_query<32, 0xc4000, 182>("tests/sstables/bigsummary", 76); } -static future do_write_sst(sstring dir, unsigned long generation) { - auto sst = make_lw_shared("ks", "cf", dir, generation, la, big); - return sst->load().then([sst, generation] { +static future do_write_sst(sstring load_dir, sstring write_dir, unsigned long generation) { + auto sst = make_lw_shared("ks", "cf", load_dir, generation, la, big); + return sst->load().then([sst, write_dir, generation] { sstables::test(sst).change_generation_number(generation + 1); + sstables::test(sst).change_dir(write_dir); auto fut = sstables::test(sst).store(); return std::move(fut).then([sst = std::move(sst)] { return make_ready_future(std::move(sst)); @@ -180,8 +182,8 @@ static future do_write_sst(sstring dir, unsigned long generation) { }); } -static future<> write_sst_info(sstring dir, unsigned long generation) { - return do_write_sst(dir, generation).then([] (auto ptr) { return make_ready_future<>(); }); +static future<> write_sst_info(sstring load_dir, sstring write_dir, unsigned long generation) { + return do_write_sst(load_dir, write_dir, generation).then([] (auto ptr) { return make_ready_future<>(); }); } using bufptr_t = std::unique_ptr; @@ -223,11 +225,12 @@ static future<> compare_files(sstdesc file1, sstdesc file2, sstable::component_t } static future<> check_component_integrity(sstable::component_type component) { - return write_sst_info("tests/sstables/compressed", 1).then([component] { + auto tmp = make_lw_shared(); + return write_sst_info("tests/sstables/compressed", tmp->path, 1).then([component, tmp] { return compare_files(sstdesc{"tests/sstables/compressed", 1 }, - sstdesc{"tests/sstables/compressed", 2 }, + sstdesc{tmp->path, 2 }, component); - }); + }).then([tmp] {}); } SEASTAR_TEST_CASE(check_compressed_info_func) { @@ -235,8 +238,9 @@ SEASTAR_TEST_CASE(check_compressed_info_func) { } SEASTAR_TEST_CASE(check_summary_func) { - return do_write_sst("tests/sstables/compressed", 1).then([] (auto sst1) { - auto sst2 = make_lw_shared("ks", "cf", "tests/sstables/compressed", 2, la, big); + auto tmp = make_lw_shared(); + return do_write_sst("tests/sstables/compressed", tmp->path, 1).then([tmp] (auto sst1) { + auto sst2 = make_lw_shared("ks", "cf", tmp->path, 2, la, big); return sstables::test(sst2).read_summary().then([sst1, sst2] { summary& sst1_s = sstables::test(sst1).get_summary(); summary& sst2_s = sstables::test(sst2).get_summary(); @@ -247,7 +251,7 @@ SEASTAR_TEST_CASE(check_summary_func) { BOOST_REQUIRE(sst1_s.first_key.value == sst2_s.first_key.value); BOOST_REQUIRE(sst1_s.last_key.value == sst2_s.last_key.value); }); - }); + }).then([tmp] {}); } SEASTAR_TEST_CASE(check_filter_func) { @@ -255,8 +259,9 @@ SEASTAR_TEST_CASE(check_filter_func) { } SEASTAR_TEST_CASE(check_statistics_func) { - return do_write_sst("tests/sstables/compressed", 1).then([] (auto sst1) { - auto sst2 = make_lw_shared("ks", "cf", "tests/sstables/compressed", 2, la, big); + auto tmp = make_lw_shared(); + return do_write_sst("tests/sstables/compressed", tmp->path, 1).then([tmp] (auto sst1) { + auto sst2 = make_lw_shared("ks", "cf", tmp->path, 2, la, big); return sstables::test(sst2).read_statistics().then([sst1, sst2] { statistics& sst1_s = sstables::test(sst1).get_statistics(); statistics& sst2_s = sstables::test(sst2).get_statistics(); @@ -271,19 +276,20 @@ SEASTAR_TEST_CASE(check_statistics_func) { }); // TODO: compare the field contents from both sstables. }); - }); + }).then([tmp] {}); } SEASTAR_TEST_CASE(check_toc_func) { - return do_write_sst("tests/sstables/compressed", 1).then([] (auto sst1) { - auto sst2 = make_lw_shared("ks", "cf", "tests/sstables/compressed", 2, la, big); + auto tmp = make_lw_shared(); + return do_write_sst("tests/sstables/compressed", tmp->path, 1).then([tmp] (auto sst1) { + auto sst2 = make_lw_shared("ks", "cf", tmp->path, 2, la, big); return sstables::test(sst2).read_toc().then([sst1, sst2] { auto& sst1_c = sstables::test(sst1).get_components(); auto& sst2_c = sstables::test(sst2).get_components(); BOOST_REQUIRE(sst1_c == sst2_c); }); - }); + }).then([tmp] {}); } SEASTAR_TEST_CASE(uncompressed_random_access_read) { diff --git a/tests/sstable_test.hh b/tests/sstable_test.hh index acf182e760..89dae20f93 100644 --- a/tests/sstable_test.hh +++ b/tests/sstable_test.hh @@ -100,6 +100,10 @@ public: _sst->_generation = generation; } + void change_dir(sstring dir) { + _sst->_dir = dir; + } + future<> store() { _sst->_components.erase(sstable::component_type::Index); _sst->_components.erase(sstable::component_type::Data);