From 30d4aaef87f5df41fffe41cfd6748f5f6a9255ab Mon Sep 17 00:00:00 2001 From: haiqi96 <14502009+haiqi96@users.noreply.github.com> Date: Fri, 7 Jun 2024 18:29:19 +0000 Subject: [PATCH] Address more code review concerns --- components/core/CMakeLists.txt | 2 +- components/core/src/clp/clp/CMakeLists.txt | 2 +- .../core/src/clp/clp/FileDecompressor.cpp | 18 ++++--- .../core/src/clp/clp/FileDecompressor.hpp | 2 +- .../core/src/clp/ir/LogEventSerializer.cpp | 48 +++++++++---------- .../core/src/clp/ir/LogEventSerializer.hpp | 8 +++- .../clp/ir/{Constant.hpp => constants.hpp} | 4 +- 7 files changed, 48 insertions(+), 36 deletions(-) rename components/core/src/clp/ir/{Constant.hpp => constants.hpp} (62%) diff --git a/components/core/CMakeLists.txt b/components/core/CMakeLists.txt index ddd9360a1..d4b088b1d 100644 --- a/components/core/CMakeLists.txt +++ b/components/core/CMakeLists.txt @@ -332,7 +332,7 @@ set(SOURCE_FILES_unitTest src/clp/GlobalSQLiteMetadataDB.hpp src/clp/Grep.cpp src/clp/Grep.hpp - src/clp/ir/Constant.hpp + src/clp/ir/constants.hpp src/clp/ir/LogEvent.hpp src/clp/ir/LogEventDeserializer.cpp src/clp/ir/LogEventDeserializer.hpp diff --git a/components/core/src/clp/clp/CMakeLists.txt b/components/core/src/clp/clp/CMakeLists.txt index b2931385c..ec9e58d37 100644 --- a/components/core/src/clp/clp/CMakeLists.txt +++ b/components/core/src/clp/clp/CMakeLists.txt @@ -36,7 +36,7 @@ set( ../GlobalMySQLMetadataDB.hpp ../GlobalSQLiteMetadataDB.cpp ../GlobalSQLiteMetadataDB.hpp - ../ir/Constant.hpp + ../ir/constants.hpp ../ir/LogEvent.hpp ../ir/LogEventDeserializer.cpp ../ir/LogEventDeserializer.hpp diff --git a/components/core/src/clp/clp/FileDecompressor.cpp b/components/core/src/clp/clp/FileDecompressor.cpp index 52b452947..a9440a624 100644 --- a/components/core/src/clp/clp/FileDecompressor.cpp +++ b/components/core/src/clp/clp/FileDecompressor.cpp @@ -4,7 +4,7 @@ #include #include -#include "../ir/Constant.hpp" +#include "../ir/constants.hpp" #include "../ir/LogEventSerializer.hpp" #include "../ir/utils.hpp" @@ -33,8 +33,10 @@ bool rename_ir_file( size_t begin_message_ix, size_t end_message_ix ) { - string ir_file_name = file_orig_id + "_" + std::to_string(begin_message_ix) + "_" - + std::to_string(end_message_ix) + ir::cIrFileExtension; + auto ir_file_name = file_orig_id + "_"; + ir_file_name += std::to_string(begin_message_ix) + "_"; + ir_file_name += std::to_string(end_message_ix) + "_"; + ir_file_name += ir::cIrFileExtension; auto const renamed_ir_path = output_directory / ir_file_name; try { @@ -167,7 +169,9 @@ bool FileDecompressor::decompress_to_ir( } boost::filesystem::path temp_ir_path{temp_output_dir}; - temp_ir_path /= m_encoded_file.get_id_as_string() + ir::cIrFileExtension; + auto temp_ir_file = m_encoded_file.get_id_as_string(); + temp_ir_file += ir::cIrFileExtension; + temp_ir_path /= temp_ir_file; auto const& file_orig_id = m_encoded_file.get_orig_file_id_as_string(); auto begin_message_ix = m_encoded_file.get_begin_message_ix(); @@ -214,10 +218,12 @@ bool FileDecompressor::decompress_to_ir( } } - if (false == ir_serializer.serialize_log_event( + if (false + == ir_serializer.serialize_log_event( m_encoded_message.get_ts_in_milli(), m_decompressed_message - )) { + )) + { SPDLOG_ERROR( "Failed to serialize log event: {} with ts {}", m_decompressed_message.c_str(), diff --git a/components/core/src/clp/clp/FileDecompressor.hpp b/components/core/src/clp/clp/FileDecompressor.hpp index c5742d400..a31ea705b 100644 --- a/components/core/src/clp/clp/FileDecompressor.hpp +++ b/components/core/src/clp/clp/FileDecompressor.hpp @@ -1,8 +1,8 @@ #ifndef CLP_CLP_FILEDECOMPRESSOR_HPP #define CLP_CLP_FILEDECOMPRESSOR_HPP -#include #include +#include #include "../FileWriter.hpp" #include "../streaming_archive/MetadataDB.hpp" diff --git a/components/core/src/clp/ir/LogEventSerializer.cpp b/components/core/src/clp/ir/LogEventSerializer.cpp index f5f5f5a0f..c3f4141c7 100644 --- a/components/core/src/clp/ir/LogEventSerializer.cpp +++ b/components/core/src/clp/ir/LogEventSerializer.cpp @@ -25,7 +25,7 @@ LogEventSerializer::~LogEventSerializer() { } template -auto LogEventSerializer::open(string const& file_path) -> ErrorCode { +auto LogEventSerializer::open(string const& file_path) -> bool { if (m_is_open) { throw OperationFailed(ErrorCode_NotReady, __FILENAME__, __LINE__); } @@ -41,24 +41,24 @@ auto LogEventSerializer::open(string const& file_path) -> Er if constexpr (std::is_same_v) { m_prev_event_timestamp = 0; res = ffi::ir_stream::four_byte_encoding::serialize_preamble( - cTimestampPattern, - cTimestampPatternSyntax, - cTimezoneID, - m_prev_event_timestamp, - m_ir_buf + cTimestampPattern, + cTimestampPatternSyntax, + cTimezoneID, + m_prev_event_timestamp, + m_ir_buf ); } else { res = clp::ffi::ir_stream::eight_byte_encoding::serialize_preamble( - cTimestampPattern, - cTimestampPatternSyntax, - cTimezoneID, - m_ir_buf + cTimestampPattern, + cTimestampPatternSyntax, + cTimezoneID, + m_ir_buf ); } if (false == res) { close_writer(); - return ErrorCode_Failure; + return true; } m_is_open = true; @@ -66,7 +66,7 @@ auto LogEventSerializer::open(string const& file_path) -> Er // Flush the preamble flush(); - return ErrorCode_Success; + return false; } template @@ -75,8 +75,8 @@ auto LogEventSerializer::flush() -> void { throw OperationFailed(ErrorCode_NotInit, __FILENAME__, __LINE__); } m_zstd_compressor.write( - size_checked_pointer_cast(m_ir_buf.data()), - m_ir_buf.size() + size_checked_pointer_cast(m_ir_buf.data()), + m_ir_buf.size() ); m_serialized_size += m_ir_buf.size(); m_ir_buf.clear(); @@ -106,19 +106,19 @@ auto LogEventSerializer::serialize_log_event( bool res{}; if constexpr (std::is_same_v) { res = clp::ffi::ir_stream::eight_byte_encoding::serialize_log_event( - timestamp, - message, - logtype, - m_ir_buf + timestamp, + message, + logtype, + m_ir_buf ); } else { auto const timestamp_delta = timestamp - m_prev_event_timestamp; m_prev_event_timestamp = timestamp; res = clp::ffi::ir_stream::four_byte_encoding::serialize_log_event( - timestamp_delta, - message, - logtype, - m_ir_buf + timestamp_delta, + message, + logtype, + m_ir_buf ); } if (false == res) { @@ -139,9 +139,9 @@ auto LogEventSerializer::close_writer() -> void { template LogEventSerializer::~LogEventSerializer(); template LogEventSerializer::~LogEventSerializer(); template auto LogEventSerializer::open(string const& file_path -) -> ErrorCode; +) -> bool; template auto LogEventSerializer::open(string const& file_path -) -> ErrorCode; +) -> bool; template auto LogEventSerializer::flush() -> void; template auto LogEventSerializer::flush() -> void; template auto LogEventSerializer::close() -> void; diff --git a/components/core/src/clp/ir/LogEventSerializer.hpp b/components/core/src/clp/ir/LogEventSerializer.hpp index f2f48e27c..54109d208 100644 --- a/components/core/src/clp/ir/LogEventSerializer.hpp +++ b/components/core/src/clp/ir/LogEventSerializer.hpp @@ -52,10 +52,14 @@ class LogEventSerializer { /** * Creates a Zstandard-compressed IR file on disk, and writes the IR file's preamble. - * * @param file_path + * @return true on success, false if the preamble fails to serialize + * @throw FileWriter::OperationFailed if the FileWriter fails to open the file specified by + * file_path + * @throw streaming_compression::zstd::Compressor if Zstandard compressor fails to open + * @throw ir::LogEventSerializer::OperationFailed on failure */ - [[nodiscard]] auto open(std::string const& file_path) -> ErrorCode; + [[nodiscard]] auto open(std::string const& file_path) -> bool; /** * Flushes any buffered data. diff --git a/components/core/src/clp/ir/Constant.hpp b/components/core/src/clp/ir/constants.hpp similarity index 62% rename from components/core/src/clp/ir/Constant.hpp rename to components/core/src/clp/ir/constants.hpp index f819827a3..2c287c2e0 100644 --- a/components/core/src/clp/ir/Constant.hpp +++ b/components/core/src/clp/ir/constants.hpp @@ -1,8 +1,10 @@ #ifndef CLP_IR_CONSTANTS_HPP #define CLP_IR_CONSTANTS_HPP +#include + namespace clp::ir { -constexpr char cIrFileExtension[] = ".clp.zst"; +constexpr std::string_view cIrFileExtension{".clp.zst"}; } // namespace clp::ir #endif // CLP_IR_CONSTANTS_HPP