From 3749148339da1d4b063df407c55bda3fefcad94c Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Sun, 31 Mar 2019 18:40:06 +0300 Subject: [PATCH] storage_service: fix handling of load_new_sstables exception ignore_ready_future in load_new_ss_tables broke migration_test:TestMigration_with_*.migrate_sstable_with_counter_test_expect_fail dtests. The java.io.NotSerializableException in nodetool was caused by exceptions that were too long. This fix prints the problematic file names onto the node system log and includes the casue in the resulting exception so to provide the user with information about the nature of the error. Fixes #4375 Signed-off-by: Benny Halevy Message-Id: <20190331154006.12808-1-bhalevy@scylladb.com> --- api/storage_service.cc | 7 ++----- distributed_loader.cc | 39 ++++++++++++++++++++++----------------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/api/storage_service.cc b/api/storage_service.cc index 6ed2344bf4..8583b19efc 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -704,11 +704,8 @@ void set_storage_service(http_context& ctx, routes& r) { return s.load_new_sstables(ks, cf); }).then_wrapped([] (auto&& f) { if (f.failed()) { - // Avoid `Exceptional future ignored` - // Unfortunately incorporating the exception string in httpd::server_error_exception - // results in `java.io.NotSerializableException` - f.ignore_ready_future(); - throw httpd::server_error_exception("Failed to load new sstables"); + auto msg = fmt::format("Failed to load new sstables: {}", f.get_exception()); + return make_exception_future(httpd::server_error_exception(msg)); } return make_ready_future(json_void()); }); diff --git a/distributed_loader.cc b/distributed_loader.cc index 731f526f41..69d2763ed7 100644 --- a/distributed_loader.cc +++ b/distributed_loader.cc @@ -179,20 +179,28 @@ static future> get_all_shared_sstables(dis }); } +template +static inline +future<> verification_error(fs::path path, const char* fstr, Args&&... args) { + auto emsg = fmt::format(fstr, std::forward(args)...); + dblog.error("{}: {}", path.string(), emsg); + return make_exception_future<>(std::runtime_error(emsg)); +} + // Verify that all files and directories are owned by current uid // and that files can be read and directories can be read, written, and looked up (execute) // No other file types may exist. future<> distributed_loader::verify_owner_and_mode(fs::path path) { return file_stat(path.string()).then([path = std::move(path)] (stat_data sd) { if (sd.uid != geteuid()) { - throw std::runtime_error(fmt::format(FMT_STRING("{} not owned by current euid: {}, owner: {}"), path, geteuid(), sd.uid)); + return verification_error(std::move(path), "File not owned by current euid: {}. Owner is: {}", geteuid(), sd.uid); } switch (sd.type) { case directory_entry_type::regular: { auto f = file_accessible(path.string(), access_flags::read); return f.then([path = std::move(path)] (bool can_access) { if (!can_access) { - throw std::runtime_error(fmt::format(FMT_STRING("File {} cannot be accessed for read"), path)); + return verification_error(std::move(path), "File cannot be accessed for read"); } return make_ready_future<>(); }); @@ -202,7 +210,7 @@ future<> distributed_loader::verify_owner_and_mode(fs::path path) { auto f = file_accessible(path.string(), access_flags::read | access_flags::write | access_flags::execute); return f.then([path = std::move(path)] (bool can_access) { if (!can_access) { - throw std::runtime_error(fmt::format(FMT_STRING("Directory {} cannot be accessed for read, write, and execute"), path)); + return verification_error(std::move(path), "Directory cannot be accessed for read, write, and execute"); } return lister::scan_dir(path, {}, [] (fs::path dir, directory_entry de) { return verify_owner_and_mode(dir / de.name); @@ -211,7 +219,7 @@ future<> distributed_loader::verify_owner_and_mode(fs::path path) { break; } default: - throw std::runtime_error(fmt::format(FMT_STRING("{} must be either a regular file or a directory (type={})"), path, int(sd.type))); + return verification_error(std::move(path), "Must be either a regular file or a directory (type={})", static_cast(sd.type)); } }); }; @@ -233,19 +241,15 @@ distributed_loader::flush_upload_dir(distributed& db, distributed flushed; auto& cf = db.local().find_column_family(ks_name, cf_name); - lister::scan_dir(fs::path(cf._config.datadir) / "upload", { directory_entry_type::regular }, - [&descriptors] (fs::path parent_dir, directory_entry de) { - return verify_owner_and_mode(parent_dir / de.name).then([&descriptors, parent_dir = std::move(parent_dir), de = std::move(de)] { - auto comps = sstables::entry_descriptor::make_descriptor(parent_dir.native(), de.name); - if (comps.component != component_type::TOC) { - return make_ready_future<>(); - } - descriptors.emplace(comps.generation, std::move(comps)); - return make_ready_future<>(); - }).handle_exception([parent_dir = std::move(parent_dir), de = std::move(de)] (auto ep) { - dblog.error("Failed owner and mode verification: {}", ep); - std::rethrow_exception(ep); - }); + auto upload_dir = fs::path(cf._config.datadir) / "upload"; + verify_owner_and_mode(upload_dir).get(); + lister::scan_dir(upload_dir, { directory_entry_type::regular }, [&descriptors] (fs::path parent_dir, directory_entry de) { + auto comps = sstables::entry_descriptor::make_descriptor(parent_dir.native(), de.name); + if (comps.component != component_type::TOC) { + return make_ready_future<>(); + } + descriptors.emplace(comps.generation, std::move(comps)); + return make_ready_future<>(); }, &column_family::manifest_json_filter).get(); flushed.reserve(descriptors.size()); @@ -264,6 +268,7 @@ distributed_loader::flush_upload_dir(distributed& db, distributed