From f4ca94d13ba7c3ea2572f9efb66f9acfdacc2bfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Chojnowski?= Date: Mon, 17 Mar 2025 16:09:18 +0100 Subject: [PATCH] compress.hh: switch compressor::name() from an instance member to a virtual call Before this patch, `compressor` is designed to be a proper abstract class, where the creator of a compressor doesn't even know what he's creating -- he passes a name, and it gets turned into a `compressor` behind a scenes. But later, when creation of compressors will involve looking up dictionaries, this abstraction will only get in the way. So we give up on keeping `compressor` abstract, and instead of using "opaque" names we turn to an explicit enum of possible compressor types. The main point of this patch is to add the `algorithm` enum and the `algorithm_to_name()` function. The rest of the patch switches the `compressor::name()` function to use `algorithm_to_name()` instead of the passed-by-constructor `compressor::_name`, to keep a single source of truth for the names. --- api/storage_service.cc | 2 +- compress.cc | 53 ++++++++++++++++++++++++++++-------------- compress.hh | 25 ++++++++++++-------- sstables/compress.cc | 2 +- zstd.cc | 11 +++++---- 5 files changed, 60 insertions(+), 33 deletions(-) diff --git a/api/storage_service.cc b/api/storage_service.cc index 99adb21a7b..345e894f99 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -1491,7 +1491,7 @@ rest_sstable_info(http_context& ctx, std::unique_ptr req) { if (!cp->options().contains(compression_parameters::SSTABLE_COMPRESSION)) { ss::mapper e; e.key = compression_parameters::SSTABLE_COMPRESSION; - e.value = cp->name(); + e.value = sstring(cp->name()); nm.attributes.push(std::move(e)); } info.extended_properties.push(std::move(nm)); diff --git a/compress.cc b/compress.cc index 56174bc4ff..660872d714 100644 --- a/compress.cc +++ b/compress.cc @@ -10,51 +10,43 @@ #include #include +#include #include "compress.hh" #include "exceptions/exceptions.hh" #include "utils/class_registrator.hh" -sstring compressor::make_name(std::string_view short_name) { - return seastar::format("org.apache.cassandra.io.compress.{}", short_name); -} +static seastar::logger compressor_factory_logger("compressor_factory"); class lz4_processor: public compressor { public: - using compressor::compressor; - size_t uncompress(const char* input, size_t input_len, char* output, size_t output_len) const override; size_t compress(const char* input, size_t input_len, char* output, size_t output_len) const override; size_t compress_max_size(size_t input_len) const override; + algorithm get_algorithm() const override; }; class snappy_processor: public compressor { public: - using compressor::compressor; - size_t uncompress(const char* input, size_t input_len, char* output, size_t output_len) const override; size_t compress(const char* input, size_t input_len, char* output, size_t output_len) const override; size_t compress_max_size(size_t input_len) const override; + algorithm get_algorithm() const override { return algorithm::snappy; } }; class deflate_processor: public compressor { public: - using compressor::compressor; - size_t uncompress(const char* input, size_t input_len, char* output, size_t output_len) const override; size_t compress(const char* input, size_t input_len, char* output, size_t output_len) const override; size_t compress_max_size(size_t input_len) const override; + algorithm get_algorithm() const override { return algorithm::deflate; } }; -compressor::compressor(sstring name) - : _name(std::move(name)) -{} - std::set compressor::option_names() const { return {}; } @@ -63,12 +55,18 @@ std::map compressor::options() const { return {}; } +std::string compressor::name() const { + auto result = std::string(compression_parameters::name_prefix); + result.append(compression_parameters::algorithm_to_name(get_algorithm())); + return result; +} + compressor::ptr_type compressor::create(const sstring& name, const opt_getter& opts) { if (name.empty()) { return {}; } - qualified_name qn(make_name(""), name); + qualified_name qn(compression_parameters::name_prefix, name); for (auto& c : { lz4, snappy, deflate }) { if (c->name() == static_cast(qn)) { @@ -93,9 +91,9 @@ shared_ptr compressor::create(const std::map& opti return {}; } -thread_local const shared_ptr compressor::lz4 = ::make_shared(make_name("LZ4Compressor")); -thread_local const shared_ptr compressor::snappy = ::make_shared(make_name("SnappyCompressor")); -thread_local const shared_ptr compressor::deflate = ::make_shared(make_name("DeflateCompressor")); +thread_local const shared_ptr compressor::lz4 = ::make_shared(); +thread_local const shared_ptr compressor::snappy = ::make_shared(); +thread_local const shared_ptr compressor::deflate = ::make_shared(); const sstring compression_parameters::SSTABLE_COMPRESSION = "sstable_compression"; const sstring compression_parameters::CHUNK_LENGTH_KB = "chunk_length_in_kb"; @@ -113,6 +111,23 @@ compression_parameters::compression_parameters(compressor_ptr c) : _compressor(std::move(c)) {} +std::string_view compression_parameters::algorithm_to_name(algorithm alg) { + switch (alg) { + case algorithm::lz4: return "LZ4Compressor"; + case algorithm::deflate: return "DeflateCompressor"; + case algorithm::snappy: return "SnappyCompressor"; + case algorithm::zstd: return "ZstdCompressor"; + case algorithm::none: on_internal_error(compressor_factory_logger, "algorithm_to_name(): called with algorithm::none"); + } + abort(); +} + +std::string compression_parameters::algorithm_to_qualified_name(algorithm alg) { + auto result = std::string(name_prefix); + result.append(algorithm_to_name(alg)); + return result; +} + compression_parameters::compression_parameters(const std::map& options) { _compressor = compressor::create(options); @@ -250,6 +265,10 @@ size_t lz4_processor::compress_max_size(size_t input_len) const { return LZ4_COMPRESSBOUND(input_len) + 4; } +auto lz4_processor::get_algorithm() const -> algorithm { + return algorithm::lz4; +} + size_t deflate_processor::uncompress(const char* input, size_t input_len, char* output, size_t output_len) const { z_stream zs; diff --git a/compress.hh b/compress.hh index 71d7da0773..391e824a5e 100644 --- a/compress.hh +++ b/compress.hh @@ -18,9 +18,14 @@ #include "seastarx.hh" class compressor { - sstring _name; public: - compressor(sstring); + enum class algorithm { + lz4, + zstd, + snappy, + deflate, + none, + }; virtual ~compressor() {} @@ -50,12 +55,9 @@ public: */ virtual std::map options() const; - /** - * Compressor class name. - */ - const sstring& name() const { - return _name; - } + std::string name() const; + + virtual algorithm get_algorithm() const = 0; // to cheaply bridge sstable compression options / maps using opt_string = std::optional; @@ -68,8 +70,6 @@ public: static thread_local const ptr_type lz4; static thread_local const ptr_type snappy; static thread_local const ptr_type deflate; - - static sstring make_name(std::string_view short_name); }; template @@ -80,6 +80,9 @@ using compressor_registry = class_registry&); }; diff --git a/sstables/compress.cc b/sstables/compress.cc index 957c72deca..a39448954b 100644 --- a/sstables/compress.cc +++ b/sstables/compress.cc @@ -295,7 +295,7 @@ size_t local_compression::compress_max_size(size_t input_len) const { void compression::set_compressor(compressor_ptr c) { if (c) { - unqualified_name uqn(compressor::make_name(""), c->name()); + unqualified_name uqn(compression_parameters::name_prefix, c->name()); const sstring& cn = uqn; name.value = bytes(cn.begin(), cn.end()); for (auto& [k, v] : c->options()) { diff --git a/zstd.cc b/zstd.cc index c066e53701..ea8c4f1f28 100644 --- a/zstd.cc +++ b/zstd.cc @@ -20,7 +20,6 @@ #include static const sstring COMPRESSION_LEVEL = "compression_level"; -static const sstring COMPRESSOR_NAME = compressor::make_name("ZstdCompressor"); static const size_t DCTX_SIZE = ZSTD_estimateDCtxSize(); class zstd_processor : public compressor { @@ -83,10 +82,10 @@ public: std::set option_names() const override; std::map options() const override; + algorithm get_algorithm() const override; }; -zstd_processor::zstd_processor(const opt_getter& opts) - : compressor(COMPRESSOR_NAME) { +zstd_processor::zstd_processor(const opt_getter& opts) { auto level = opts(COMPRESSION_LEVEL); if (level) { try { @@ -152,5 +151,9 @@ std::map zstd_processor::options() const { return {{COMPRESSION_LEVEL, std::to_string(_compression_level)}}; } +auto zstd_processor::get_algorithm() const -> algorithm { + return algorithm::zstd; +} + static const class_registrator - registrator(COMPRESSOR_NAME); + registrator(sstring(compression_parameters::algorithm_to_name(compressor::algorithm::zstd)));