From 77169e26471d2c1844dfdb73c458940a7008d9bd Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 24 Mar 2023 20:20:34 +0300 Subject: [PATCH 1/2] sstables: Relax exception handling in do_write_simple This effectively reverts 000514e7cc (sstable: close file_writer if an exception in thrown) because it became obsoleted by 60873d2360 (sstable: file_writer: auto-close in destructor). The change is in fact idempotent. Before the patch writer was closed regardless of write/flush failing or not. After the patch writer will close itself in destrictor for sure. Before the patch an exception from write/flush was caught, then close was called and regardless of close failed or not the former exception was re-thrown. After the patch an exception from write/flush will result inin writer destruction that would ignore close exception (if any). Before the patch throwing close after successfull write/flush re-threw the close exception. After the patch writer will be closed "by hand" and any exception will be reported. Signed-off-by: Pavel Emelyanov --- sstables/sstables.cc | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index db3d1bdcde..99c33beb94 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -1061,25 +1061,9 @@ void sstable::do_write_simple(component_type type, const io_priority_class& pc, options.io_priority_class = pc; auto w = make_component_file_writer(type, std::move(options)).get0(); std::exception_ptr eptr; - try { - write_component(_version, w); - w.flush(); - } catch (...) { - eptr = std::current_exception(); - } - try { - w.close(); - } catch (...) { - std::exception_ptr close_eptr = std::current_exception(); - sstlog.warn("failed to close file_writer: {}", close_eptr); - // If write succeeded but close failed, we rethrow close's exception. - if (!eptr) { - eptr = close_eptr; - } - } - if (eptr) { - std::rethrow_exception(eptr); - } + write_component(_version, w); + w.flush(); + w.close(); } template From 886a1392a83fa4cfe2cf76710b367f0480cfbf33 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 24 Mar 2023 20:35:26 +0300 Subject: [PATCH 2/2] sstables: Move writer flush into close (and remove it) Writing into sstable component output stream should be done with care. In particular -- flushing can happen only once right before closing the stream. Flushing the stream in between several writes is not going to work, because file stream would step on unaligned IO and S3 upload stream would send completion message to the server and would lose any subsequent write. Having said that, it's better to remove the flush() ability from the component writer not to tempt the developers. refs: #13320 Signed-off-by: Pavel Emelyanov --- sstables/sstables.cc | 22 ++++++++++++++++++---- sstables/writer.hh | 5 ----- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/sstables/sstables.cc b/sstables/sstables.cc index 99c33beb94..bca7055003 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -910,14 +910,31 @@ file_writer::~file_writer() { } void file_writer::close() { + // Writing into sstable component output stream should be done with care. + // In particular -- flushing can happen only once right before closing + // the stream. Flushing the stream in between several writes is not going + // to work, because file stream would step on unaligned IO and S3 upload + // stream would send completion message to the server and would lose any + // subsequent write. assert(!_closed && "file_writer already closed"); + std::exception_ptr ex; + try { + _out.flush().get(); + } catch (...) { + ex = std::current_exception(); + } try { _closed = true; _out.close().get(); } catch (...) { auto e = std::current_exception(); sstlog.error("Error while closing {}: {}", get_filename(), e); - std::rethrow_exception(e); + if (!ex) { + ex = std::move(e); + } + } + if (ex) { + std::rethrow_exception(std::move(ex)); } } @@ -970,7 +987,6 @@ void sstable::filesystem_storage::open(sstable& sst, const io_priority_class& pc bytes b = bytes(reinterpret_cast(value.c_str()), value.size()); write(sst._version, w, b); } - w.flush(); w.close(); // Flushing parent directory to guarantee that temporary TOC file reached @@ -1062,7 +1078,6 @@ void sstable::do_write_simple(component_type type, const io_priority_class& pc, auto w = make_component_file_writer(type, std::move(options)).get0(); std::exception_ptr eptr; write_component(_version, w); - w.flush(); w.close(); } @@ -1318,7 +1333,6 @@ void sstable::rewrite_statistics(const io_priority_class& pc) { auto w = make_component_file_writer(component_type::TemporaryStatistics, std::move(options), open_flags::wo | open_flags::create | open_flags::truncate).get0(); write(_version, w, _components->statistics); - w.flush(); w.close(); // rename() guarantees atomicity when renaming a file into place. sstable_write_io_check(rename_file, file_path, filename(component_type::Statistics)).get(); diff --git a/sstables/writer.hh b/sstables/writer.hh index 0e2aab2317..b4be7d9584 100644 --- a/sstables/writer.hh +++ b/sstables/writer.hh @@ -71,10 +71,6 @@ public: _out.write(reinterpret_cast(s.begin()), s.size()).get(); } // Must be called in a seastar thread. - void flush() { - _out.flush().get(); - } - // Must be called in a seastar thread. void close(); uint64_t offset() const { @@ -131,7 +127,6 @@ serialized_size(sstable_version_types v, const T& object) { uint64_t size = 0; auto writer = file_writer(make_sizing_output_stream(size)); write(v, writer, object); - writer.flush(); writer.close(); return size; }