From d9ba5423bbbfb8b2fd060de17e288dded24f77fc Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Mon, 22 Jul 2024 17:53:02 +0300 Subject: [PATCH] Merge 'config: round-trip boolean configuration variables' from Avi Kivity When you SELECT a boolean from system.config, it reads as true/false, but this isn't accepted on UPDATE (instead, we accept 1/0). This is surprising and annoying, so accept true/false in both directions. Not a regression, so a backport isn't strictly necessary. Closes scylladb/scylladb#19792 * github.com:scylladb/scylladb: config: specialize from-string conversion for bool config: wrap boost::lexical_cast<> when converting from strings (cherry picked from commit 9eb47b3ef0d87e1f409ec3e4a4d41ff1f6e68754) --- db/config.cc | 17 ++++++++++++++++- test/cql-pytest/test_virtual_tables.py | 24 ++++++++++++++++++++++++ utils/config_file_impl.hh | 22 +++++++++++++++++----- 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/db/config.cc b/db/config.cc index cfb25e44f2..81974dce77 100644 --- a/db/config.cc +++ b/db/config.cc @@ -99,6 +99,21 @@ error_injection_list_to_json(const std::vector +bool +config_from_string(std::string_view value) { + // boost::lexical_cast doesn't accept true/false, which are our output representations + // for bools. We want round-tripping, so we need to accept true/false. For backward + // compatibility, we also accept 1/0. #19791. + if (value == "true" || value == "1") { + return true; + } else if (value == "false" || value == "0") { + return false; + } else { + throw boost::bad_lexical_cast(typeid(std::string_view), typeid(bool)); + } +} + template <> const config_type config_type_for = config_type("bool", value_to_json); @@ -177,7 +192,7 @@ struct convert { if (!convert::decode(node, tmp)) { return false; } - rhs = boost::lexical_cast(tmp); + rhs = utils::config_from_string(tmp); return true; } }; diff --git a/test/cql-pytest/test_virtual_tables.py b/test/cql-pytest/test_virtual_tables.py index cbb665da1b..c8cb712373 100644 --- a/test/cql-pytest/test_virtual_tables.py +++ b/test/cql-pytest/test_virtual_tables.py @@ -86,6 +86,30 @@ def test_system_config_read(scylla_only, cql): assert isinstance(obj, str) assert obj and obj.isascii() and obj.isprintable() +# Verify that boolean configuration items round-trip and use the yaml/json +# representation (true/false). #19791. +def test_system_config_update_boolean(scylla_only, cql): + var = 'compaction_enforce_min_threshold' + value = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value + assert value in ('true', 'false') + other = 'true' if value == 'false' else 'false' + cql.execute(f"UPDATE system.config SET value = '{other}' WHERE name = '{var}'") + readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value + assert readback == other + + # just for completeness, check that writing 0/1 works too + cql.execute(f"UPDATE system.config SET value = '0' WHERE name = '{var}'") + readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value + assert readback == 'false' + cql.execute(f"UPDATE system.config SET value = '1' WHERE name = '{var}'") + readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value + assert readback == 'true' + + # restore original + cql.execute(f"UPDATE system.config SET value = '{value}' WHERE name = '{var}'") + readback = cql.execute(f"SELECT value FROM system.config WHERE name = '{var}'")[0].value + assert readback == value + def test_token_ring_vnodes(scylla_only, cql, test_keyspace_vnodes): rows = list(cql.execute(f"SELECT * FROM system.token_ring WHERE keyspace_name = '{test_keyspace_vnodes}'")) num_tokens = int(list(cql.execute("SELECT value FROM system.config WHERE name = 'num_tokens'"))[0].value) diff --git a/utils/config_file_impl.hh b/utils/config_file_impl.hh index b3f83fb763..6307654641 100644 --- a/utils/config_file_impl.hh +++ b/utils/config_file_impl.hh @@ -22,6 +22,18 @@ #include +namespace utils { + +template +T config_from_string(std::string_view string_representation) { + return boost::lexical_cast(string_representation); +} + +template <> +bool config_from_string(std::string_view string_representation); + +} + namespace YAML { /* @@ -84,7 +96,7 @@ std::istream& operator>>(std::istream& is, std::unordered_map& ma is >> tmp; for (auto& p : tmp) { - map[boost::lexical_cast(p.first)] = boost::lexical_cast(p.second); + map[utils::config_from_string(p.first)] = utils::config_from_string(p.second); } return is; } @@ -94,7 +106,7 @@ std::istream& operator>>(std::istream& is, std::vector& dst) { std::vector tmp; is >> tmp; for (auto& v : tmp) { - dst.emplace_back(boost::lexical_cast(v)); + dst.emplace_back(utils::config_from_string(v)); } return is; } @@ -126,7 +138,7 @@ void validate(boost::any& out, const std::vector& in, std::unordere ve = s.begin() + i->position(); } - (*p)[boost::lexical_cast(k)] = boost::lexical_cast(sstring(vs, ve)); + (*p)[utils::config_from_string(k)] = utils::config_from_string(sstring(vs, ve)); } } } @@ -210,7 +222,7 @@ bool utils::config_file::named_value::set_value(sstring value, config_source return false; } - (*this)(boost::lexical_cast(value), src); + (*this)(config_from_string(value), src); return true; } @@ -232,7 +244,7 @@ future utils::config_file::named_value::set_value_on_all_shards(sstring co_return false; } - co_await smp::invoke_on_all([this, value = boost::lexical_cast(value), src] () { + co_await smp::invoke_on_all([this, value = config_from_string(value), src] () { (*this)(value, src); }); co_return true;