bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

config: fix printing of experimental feature list

Browse Source 
Recently we noticed a regression where with certain versions of the fmt
library,

   SELECT value FROM system.config WHERE name = 'experimental_features'

returns string numbers, like "5", instead of feature names like "raft".

It turns out that the fmt library keep changing their overload resolution
order when there are several ways to print something. For enum_option<T> we
happen to have to conflicting ways to print it:
  1. We have an explicit operator<<.
  2. We have an *implicit* convertor to the type held by T.

We were hoping that the operator<< always wins. But in fmt 8.1, there is
special logic that if the type is convertable to an int, this is used
before operator<<()! For experimental_features_t, the type held in it was
an old-style enum, so it is indeed convertible to int.

The solution I used in this patch is to replace the old-style enum
in experimental_features_t by the newer and more recommended "enum class",
which does not have an implicit conversion to int.

I could have fixed it in other ways, but it wouldn't have been much
prettier. For example, dropping the implicit convertor would require
us to change a bunch of switch() statements over enum_option (and
not just experimental_features_t, but other types of enum_option).

Going forward, all uses of enum_option should use "enum class", not
"enum". tri_mode_restriction_t was already using an enum class, and
now so does experimental_features_t. I changed the examples in the
comments to also use "enum class" instead of enum.

This patch also adds to the existing experimental_features test a
check that the feature names are words that are not numbers.

Fixes #11003.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes #11004
This commit is contained in:
Nadav Har'El
2022-07-10 13:40:45 +03:00
committed by Piotr Sarna
parent 4a4d9ec9c0
commit cc69177dcc
12 changed files with 36 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
db/config.cc
View File

@@ -253,7 +253,7 @@ static db::tri_mode_restriction_t::mode strict_allow_filtering_default() {
static std::vector<sstring> experimental_feature_names() {
std::vector<sstring> ret;
for (const auto& f : db::experimental_features_t::map()) {
if (f.second != db::experimental_features_t::UNUSED) {
if (f.second != db::experimental_features_t::feature::UNUSED) {
ret.push_back(f.first);
}
}
@@ -1009,8 +1009,8 @@ db::fs::path db::config::get_conf_sub(db::fs::path sub) {
bool db::config::check_experimental(experimental_features_t::feature f) const {
if (experimental()
&& f != experimental_features_t::UNUSED
&& f != experimental_features_t::RAFT) {
&& f != experimental_features_t::feature::UNUSED
&& f != experimental_features_t::feature::RAFT) {
return true;
}
const auto& optval = experimental_features();
@@ -1047,20 +1047,20 @@ std::unordered_map<sstring, db::experimental_features_t::feature> db::experiment
// to UNUSED switch for a while, and can be eventually
// removed altogether.
return {
{"lwt", UNUSED},
{"udf", UDF},
{"cdc", UNUSED},
{"alternator-streams", ALTERNATOR_STREAMS},
{"alternator-ttl", ALTERNATOR_TTL},
{"raft", RAFT},
{"keyspace-storage-options", KEYSPACE_STORAGE_OPTIONS},
{"lwt", feature::UNUSED},
{"udf", feature::UDF},
{"cdc", feature::UNUSED},
{"alternator-streams", feature::ALTERNATOR_STREAMS},
{"alternator-ttl", feature::ALTERNATOR_TTL},
{"raft", feature::RAFT},
{"keyspace-storage-options", feature::KEYSPACE_STORAGE_OPTIONS},
};
}
std::vector<enum_option<db::experimental_features_t>> db::experimental_features_t::all() {
std::vector<enum_option<db::experimental_features_t>> ret;
for (const auto& f : db::experimental_features_t::map()) {
if (f.second != db::experimental_features_t::UNUSED) {
if (f.second != db::experimental_features_t::feature::UNUSED) {
ret.push_back(f.second);
}
}

2
db/config.hh
View File

@@ -83,7 +83,7 @@ namespace db {
struct experimental_features_t {
// NOTE: RAFT feature is not enabled via `experimental` umbrella flag.
// This option should be enabled explicitly.
enum feature { UNUSED, UDF, ALTERNATOR_STREAMS, ALTERNATOR_TTL, RAFT,
enum class feature { UNUSED, UDF, ALTERNATOR_STREAMS, ALTERNATOR_TTL, RAFT,
KEYSPACE_STORAGE_OPTIONS };
static std::unordered_map<sstring, feature> map(); // See enum_option.
static std::vector<enum_option<experimental_features_t>> all();

2
db/system_keyspace.cc
View File

@@ -2685,7 +2685,7 @@ std::vector<schema_ptr> system_keyspace::all_tables(const db::config& cfg) {
v3::truncated(),
v3::cdc_local(),
});
if (cfg.check_experimental(db::experimental_features_t::RAFT)) {
if (cfg.check_experimental(db::experimental_features_t::feature::RAFT)) {
r.insert(r.end(), {raft(), raft_snapshots(), raft_config(), group0_history(), discovery()});
}
// legacy schema

10
gms/feature_service.cc
View File

@@ -50,20 +50,20 @@ feature_config feature_config_from_db_config(db::config& cfg, std::set<sstring>
if (!cfg.enable_user_defined_functions()) {
fcfg._disabled_features.insert("UDF");
} else {
if (!cfg.check_experimental(db::experimental_features_t::UDF)) {
if (!cfg.check_experimental(db::experimental_features_t::feature::UDF)) {
throw std::runtime_error(
"You must use both enable_user_defined_functions and experimental_features:udf "
"to enable user-defined functions");
}
}
if (!cfg.check_experimental(db::experimental_features_t::ALTERNATOR_STREAMS)) {
if (!cfg.check_experimental(db::experimental_features_t::feature::ALTERNATOR_STREAMS)) {
fcfg._disabled_features.insert("ALTERNATOR_STREAMS"s);
}
if (!cfg.check_experimental(db::experimental_features_t::ALTERNATOR_TTL)) {
if (!cfg.check_experimental(db::experimental_features_t::feature::ALTERNATOR_TTL)) {
fcfg._disabled_features.insert("ALTERNATOR_TTL"s);
}
if (!cfg.check_experimental(db::experimental_features_t::RAFT)) {
if (!cfg.check_experimental(db::experimental_features_t::feature::RAFT)) {
fcfg._disabled_features.insert("SUPPORTS_RAFT_CLUSTER_MANAGEMENT"s);
fcfg._disabled_features.insert("USES_RAFT_CLUSTER_MANAGEMENT"s);
} else {
@@ -73,7 +73,7 @@ feature_config feature_config_from_db_config(db::config& cfg, std::set<sstring>
// advertised via gossip ahead of time.
fcfg._masked_features.insert("USES_RAFT_CLUSTER_MANAGEMENT"s);
}
if (!cfg.check_experimental(db::experimental_features_t::KEYSPACE_STORAGE_OPTIONS)) {
if (!cfg.check_experimental(db::experimental_features_t::feature::KEYSPACE_STORAGE_OPTIONS)) {
fcfg._disabled_features.insert("KEYSPACE_STORAGE_OPTIONS"s);
}

6
main.cc
View File

@@ -1012,7 +1012,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl
// #293 - do not stop anything
// engine().at_exit([&proxy] { return proxy.stop(); });
raft_gr.start(cfg->check_experimental(db::experimental_features_t::RAFT),
raft_gr.start(cfg->check_experimental(db::experimental_features_t::feature::RAFT),
std::ref(messaging), std::ref(gossiper), std::ref(fd)).get();
// gropu0 client exists only on shard 0
@@ -1033,7 +1033,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl
auto stop_raft = defer_verbose_shutdown("Raft", [&raft_gr] {
raft_gr.stop().get();
});
if (cfg->check_experimental(db::experimental_features_t::RAFT)) {
if (cfg->check_experimental(db::experimental_features_t::feature::RAFT)) {
supervisor::notify("starting Raft Group Registry service");
}
raft_gr.invoke_on_all(&service::raft_group_registry::start).get();
@@ -1543,7 +1543,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl
// only Alternator uses it for its TTL feature. But in the
// future if we add a CQL interface to it, we may want to
// start this outside the Alternator if().
if (cfg->check_experimental(db::experimental_features_t::ALTERNATOR_TTL)) {
if (cfg->check_experimental(db::experimental_features_t::feature::ALTERNATOR_TTL)) {
supervisor::notify("starting the expiration service");
es.start(seastar::sharded_parameter([] (const replica::database& db) { return db.as_data_dictionary(); }, std::ref(db)),
std::ref(proxy)).get();

2
service/storage_service.cc
View File

@@ -429,7 +429,7 @@ future<> storage_service::join_token_ring(cdc::generation_service& cdc_gen_servi
// - it's a fresh node start (in a fresh cluster)
// - it's a restart of an existing node, which have already joined some group0
const bool can_join_with_raft =
_db.local().get_config().check_experimental(db::experimental_features_t::RAFT) && (
_db.local().get_config().check_experimental(db::experimental_features_t::feature::RAFT) && (
_sys_ks.local().bootstrap_needed() ||
!(co_await _sys_ks.local().get_raft_group0_id()).is_null());
if (can_join_with_raft) {

4
test/boost/config_test.cc
View File

@@ -911,8 +911,8 @@ SEASTAR_TEST_CASE(test_parse_broken) {
return make_ready_future<>();
}
using ef = experimental_features_t;
using features = std::vector<enum_option<ef>>;
using ef = experimental_features_t::feature;
using features = std::vector<enum_option<experimental_features_t>>;
SEASTAR_TEST_CASE(test_parse_experimental_features_cdc) {
auto cfg_ptr = std::make_unique<config>();

2
test/boost/schema_change_test.cc
View File

@@ -640,7 +640,7 @@ future<> test_schema_digest_does_not_change_with_disabled_features(sstring data_
auto db_cfg_ptr = ::make_shared<db::config>(std::move(extensions));
auto& db_cfg = *db_cfg_ptr;
db_cfg.enable_user_defined_functions({true}, db::config::config_source::CommandLine);
db_cfg.experimental_features({experimental_features_t::UDF, experimental_features_t::KEYSPACE_STORAGE_OPTIONS}, db::config::config_source::CommandLine);
db_cfg.experimental_features({experimental_features_t::feature::UDF, experimental_features_t::feature::KEYSPACE_STORAGE_OPTIONS}, db::config::config_source::CommandLine);
if (regenerate) {
db_cfg.data_file_directories({data_dir}, db::config::config_source::CommandLine);
} else {

4
test/boost/user_function_test.cc
View File

@@ -55,7 +55,7 @@ static future<> with_udf_enabled(Func&& func) {
// Raise timeout to survive debug mode and contention, but keep in
// mind that some tests expect timeout.
db_cfg.user_defined_function_time_limit_ms(1000);
db_cfg.experimental_features({db::experimental_features_t::UDF}, db::config::config_source::CommandLine);
db_cfg.experimental_features({db::experimental_features_t::feature::UDF}, db::config::config_source::CommandLine);
return do_with_cql_env_thread(std::forward<Func>(func), db_cfg_ptr);
}
@@ -985,7 +985,7 @@ SEASTAR_THREAD_TEST_CASE(test_user_function_db_init) {
db_cfg.data_file_directories({data_dir.path().string()}, db::config::config_source::CommandLine);
db_cfg.enable_user_defined_functions({true}, db::config::config_source::CommandLine);
db_cfg.experimental_features({db::experimental_features_t::UDF}, db::config::config_source::CommandLine);
db_cfg.experimental_features({db::experimental_features_t::feature::UDF}, db::config::config_source::CommandLine);
do_with_cql_env_thread([] (cql_test_env& e) {
e.execute_cql("CREATE FUNCTION my_func(a int, b float) CALLED ON NULL INPUT RETURNS int LANGUAGE Lua AS 'return 2';").get();

7
test/cql-pytest/test_virtual_tables.py
View File

@@ -59,7 +59,8 @@ def test_versions(scylla_only, cql):
# parameters. As we noticed in issue #10047, each type of configuration
# parameter can have a different function for printing it out, and some of
# those may be wrong so we want to check as many as we can - including
# specifically the experimental_features option which was wrong in #10047.
# specifically the experimental_features option which was wrong in #10047
# and #11003.
def test_system_config_read(scylla_only, cql):
# All rows should have the columns name, source, type and value:
rows = list(cql.execute("SELECT name, source, type, value FROM system.config"))
@@ -68,12 +69,14 @@ def test_system_config_read(scylla_only, cql):
values[row.name] = row.value
# Check that experimental_features exists and makes sense.
# It needs to be a JSON-formatted strings, and the strings need to be
# ASCII feature names - not binary garbage as it was in #10047.
# ASCII feature names - not binary garbage as it was in #10047,
# and not numbers-formatted-as-string as in #11003.
assert 'experimental_features' in values
obj = json.loads(values['experimental_features'])
assert isinstance(obj, list)
assert isinstance(obj[0], str)
assert obj[0] and obj[0].isascii() and obj[0].isprintable()
assert not obj[0].isnumeric() # issue #11003
# Check formatting of tri_mode_restriction like
# restrict_replication_simplestrategy. These need to be one of
# allowed string values 0, 1, true, false or warn - but in particular

4
test/lib/cql_test_env.cc
View File

@@ -616,7 +616,7 @@ public:
fd.stop().get();
});
raft_gr.start(cfg->check_experimental(db::experimental_features_t::RAFT),
raft_gr.start(cfg->check_experimental(db::experimental_features_t::feature::RAFT),
std::ref(ms), std::ref(gossiper), std::ref(fd)).get();
auto stop_raft_gr = deferred_stop(raft_gr);
raft_gr.invoke_on_all(&service::raft_group_registry::start).get();
@@ -915,7 +915,7 @@ reader_permit make_reader_permit(cql_test_env& env) {
cql_test_config raft_cql_test_config() {
cql_test_config c;
c.db_config->experimental_features({db::experimental_features_t::RAFT});
c.db_config->experimental_features({db::experimental_features_t::feature::RAFT});
return c;
}

4
utils/enum_option.hh
View File

@@ -44,11 +44,11 @@ concept HasMapInterface = requires(T t) {
/// Example:
///
/// struct Type {
/// enum ty { a1, a2, b1 };
/// enum class ty { a1, a2, b1 };
/// static unordered_map<string, ty> map();
/// };
/// unordered_map<string, Type::ty> Type::map() {
/// return {{"a1", Type::a1}, {"a2", Type::a2}, {"b1", Type::b1}};
/// return {{"a1", Type::ty::a1}, {"a2", Type::ty::a2}, {"b1", Type::ty::b1}};
/// }
/// int main(int ac, char* av[]) {
/// namespace po = boost::program_options;