From e57192517b76515692a45726df7a0e071a7e6542 Mon Sep 17 00:00:00 2001 From: Twice Date: Sun, 28 Jul 2024 12:19:01 +0900 Subject: [PATCH] feat(config): implement integer config option with unit (#2448) --- kvrocks.conf | 1 + src/common/bit_util.h | 18 +++++ src/common/parse_util.cc | 20 ++---- src/config/config.cc | 8 ++- src/config/config_type.h | 92 +++++++++++++++++++++++++ src/types/redis_stream.cc | 3 +- tests/gocase/unit/config/config_test.go | 6 ++ 7 files changed, 130 insertions(+), 18 deletions(-) diff --git a/kvrocks.conf b/kvrocks.conf index 4d953c38d4d..11f11fb6363 100644 --- a/kvrocks.conf +++ b/kvrocks.conf @@ -63,6 +63,7 @@ repl-namespace-enabled no # By default, the max length of bulk string is limited to 512MB. If you want to # change this limit to a different value(must >= 1MiB), you can use the following configuration. +# It can be just an integer (e.g. 10000000), or an integer followed by a unit (e.g. 12M, 7G, 2T). # # proto-max-bulk-len 536870912 diff --git a/src/common/bit_util.h b/src/common/bit_util.h index 9c23d78bb8d..6663ed956be 100644 --- a/src/common/bit_util.h +++ b/src/common/bit_util.h @@ -20,6 +20,14 @@ #pragma once +#include +#include +#include + +#include "encoding.h" +#include "status.h" +#include "type_util.h" + namespace util { /* Count number of bits set in the binary array pointed by 's' and long @@ -146,4 +154,14 @@ inline int64_t RawBitpos(const uint8_t *c, int64_t count, bool bit) { } // namespace msb +// num << bit <= MAX -> num <= MAX >> bit +template +StatusOr CheckedShiftLeft(T num, U bit) { + if (num <= std::numeric_limits::max() >> bit) { + return num << bit; + } + + return {Status::NotOK, "arithmetic overflow"}; +} + } // namespace util diff --git a/src/common/parse_util.cc b/src/common/parse_util.cc index 852c80f328d..a7359c78adb 100644 --- a/src/common/parse_util.cc +++ b/src/common/parse_util.cc @@ -22,15 +22,7 @@ #include -// num << bit <= MAX -> num <= MAX >> bit -template -StatusOr CheckedShiftLeft(T num, U bit) { - if (num <= std::numeric_limits::max() >> bit) { - return num << bit; - } - - return {Status::NotOK, "arithmetic overflow"}; -} +#include "bit_util.h" StatusOr ParseSizeAndUnit(const std::string &v) { auto [num, rest] = GET_OR_RET(TryParseInt(v.c_str(), 10)); @@ -38,15 +30,15 @@ StatusOr ParseSizeAndUnit(const std::string &v) { if (*rest == 0) { return num; } else if (util::EqualICase(rest, "k")) { - return CheckedShiftLeft(num, 10); + return util::CheckedShiftLeft(num, 10); } else if (util::EqualICase(rest, "m")) { - return CheckedShiftLeft(num, 20); + return util::CheckedShiftLeft(num, 20); } else if (util::EqualICase(rest, "g")) { - return CheckedShiftLeft(num, 30); + return util::CheckedShiftLeft(num, 30); } else if (util::EqualICase(rest, "t")) { - return CheckedShiftLeft(num, 40); + return util::CheckedShiftLeft(num, 40); } else if (util::EqualICase(rest, "p")) { - return CheckedShiftLeft(num, 50); + return util::CheckedShiftLeft(num, 50); } return {Status::NotOK, "encounter unexpected unit"}; diff --git a/src/config/config.cc b/src/config/config.cc index 58a155949e5..285130df26a 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -187,7 +188,8 @@ Config::Config() { {"redis-cursor-compatible", false, new YesNoField(&redis_cursor_compatible, true)}, {"resp3-enabled", false, new YesNoField(&resp3_enabled, false)}, {"repl-namespace-enabled", false, new YesNoField(&repl_namespace_enabled, false)}, - {"proto-max-bulk-len", false, new UInt64Field(&proto_max_bulk_len, 512 * MiB, 1 * MiB, UINT64_MAX)}, + {"proto-max-bulk-len", false, + new IntWithUnitField(&proto_max_bulk_len, std::to_string(512 * MiB), 1 * MiB, UINT64_MAX)}, {"json-max-nesting-depth", false, new IntField(&json_max_nesting_depth, 1024, 0, INT_MAX)}, {"json-storage-format", false, new EnumField(&json_storage_format, json_storage_formats, JsonStorageFormat::JSON)}, @@ -885,7 +887,7 @@ Status Config::Set(Server *srv, std::string key, const std::string &value) { if (!s.IsOK()) return s.Prefixed("invalid value"); } - auto origin_value = field->ToString(); + auto origin_value = field->ToStringForRewrite(); auto s = field->Set(value); if (!s.IsOK()) return s.Prefixed("failed to set new value"); @@ -922,7 +924,7 @@ Status Config::Rewrite(const std::map &tokens) { // so skip it here to avoid rewriting it as new item. continue; } - new_config[iter.first] = iter.second->ToString(); + new_config[iter.first] = iter.second->ToStringForRewrite(); } std::string namespace_prefix = "namespace."; diff --git a/src/config/config_type.h b/src/config/config_type.h index 2b0d2debfb3..299e1459789 100644 --- a/src/config/config_type.h +++ b/src/config/config_type.h @@ -29,6 +29,7 @@ #include #include +#include "bit_util.h" #include "parse_util.h" #include "status.h" #include "string_util.h" @@ -62,6 +63,7 @@ class ConfigField { virtual ~ConfigField() = default; virtual std::string Default() const = 0; virtual std::string ToString() const = 0; + virtual std::string ToStringForRewrite() const { return ToString(); } virtual Status Set(const std::string &v) = 0; virtual Status ToNumber(int64_t *n) const { return {Status::NotOK, "not supported"}; } virtual Status ToBool(bool *b) const { return {Status::NotOK, "not supported"}; } @@ -127,6 +129,7 @@ class IntegerField : public ConfigField { public: IntegerField(IntegerType *receiver, IntegerType n, IntegerType min, IntegerType max) : receiver_(receiver), default_(n), min_(min), max_(max) { + CHECK(min <= n && n <= max); *receiver_ = n; } ~IntegerField() override = default; @@ -251,3 +254,92 @@ class EnumField : public ConfigField { return {}; } }; + +enum class IntUnit { None = 0, K = 10, M = 20, G = 30, T = 40, P = 50 }; + +template +class IntWithUnitField : public ConfigField { + public: + IntWithUnitField(T *receiver, std::string default_val, T min, T max) + : receiver_(receiver), default_val_(std::move(default_val)), min_(min), max_(max) { + CHECK(ReadFrom(default_val_)); + } + + std::string Default() const override { return default_val_; } + std::string ToString() const override { return std::to_string(*receiver_); } + std::string ToStringForRewrite() const override { return ToString(*receiver_, current_unit_); } + + Status ToNumber(int64_t *n) const override { + *n = static_cast(*receiver_); + return Status::OK(); + } + + Status Set(const std::string &v) override { return ReadFrom(v); } + + Status ReadFrom(const std::string &val) { + auto [num, rest] = GET_OR_RET(TryParseInt(val.c_str(), 10)); + + if (*rest == 0) { + *receiver_ = num; + current_unit_ = IntUnit::None; + } else if (util::EqualICase(rest, "k")) { + *receiver_ = GET_OR_RET(util::CheckedShiftLeft(num, 10)); + current_unit_ = IntUnit::K; + } else if (util::EqualICase(rest, "m")) { + *receiver_ = GET_OR_RET(util::CheckedShiftLeft(num, 20)); + current_unit_ = IntUnit::M; + } else if (util::EqualICase(rest, "g")) { + *receiver_ = GET_OR_RET(util::CheckedShiftLeft(num, 30)); + current_unit_ = IntUnit::G; + } else if (util::EqualICase(rest, "t")) { + *receiver_ = GET_OR_RET(util::CheckedShiftLeft(num, 40)); + current_unit_ = IntUnit::T; + } else if (util::EqualICase(rest, "p")) { + *receiver_ = GET_OR_RET(util::CheckedShiftLeft(num, 50)); + current_unit_ = IntUnit::P; + } else { + return {Status::NotOK, fmt::format("encounter unexpected unit: `{}`", rest)}; + } + + if (min_ > *receiver_ || max_ < *receiver_) { + return {Status::NotOK, fmt::format("this config value should be between {} and {}", min_, max_)}; + } + + return Status::OK(); + } + + static std::string ToString(T val, IntUnit current_unit) { + std::string unit_str; + + switch (current_unit) { + case IntUnit::None: + unit_str = ""; + break; + case IntUnit::K: + unit_str = "K"; + break; + case IntUnit::M: + unit_str = "M"; + break; + case IntUnit::G: + unit_str = "G"; + break; + case IntUnit::T: + unit_str = "T"; + break; + case IntUnit::P: + unit_str = "P"; + break; + } + + return std::to_string(val >> int(current_unit)) + unit_str; + } + + private: + // NOTE: the receiver here will get the converted integer (e.g. "10k" -> get 10240) + T *receiver_; + std::string default_val_; + T min_; + T max_; + IntUnit current_unit_ = IntUnit::None; +}; diff --git a/src/types/redis_stream.cc b/src/types/redis_stream.cc index 4df2a2ec1d4..40f42012ac1 100644 --- a/src/types/redis_stream.cc +++ b/src/types/redis_stream.cc @@ -1759,7 +1759,8 @@ rocksdb::Status Stream::GetPendingEntries(StreamPendingOptions &options, StreamG } if (options.with_count) { - ext_results.push_back({entry_id, pel_entry.last_delivery_time_ms, pel_entry.last_delivery_count, consumer_name}); + ext_results.push_back( + {entry_id, {pel_entry.last_delivery_time_ms, pel_entry.last_delivery_count, consumer_name}}); ext_result_count++; continue; } diff --git a/tests/gocase/unit/config/config_test.go b/tests/gocase/unit/config/config_test.go index a902bb89a61..0c434e72c55 100644 --- a/tests/gocase/unit/config/config_test.go +++ b/tests/gocase/unit/config/config_test.go @@ -266,6 +266,12 @@ func TestChangeProtoMaxBulkLen(t *testing.T) { require.NoError(t, err) require.EqualValues(t, "2097152", vals["proto-max-bulk-len"]) + // change to 100MB + require.NoError(t, rdb.ConfigSet(ctx, "proto-max-bulk-len", "100M").Err()) + vals, err = rdb.ConfigGet(ctx, "proto-max-bulk-len").Result() + require.NoError(t, err) + require.EqualValues(t, "104857600", vals["proto-max-bulk-len"]) + // Must be >= 1MB require.Error(t, rdb.ConfigSet(ctx, "proto-max-bulk-len", "1024").Err()) }