From 91e677c6c8f1294029b02af29cd84e38d123648d Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Tue, 8 Aug 2023 13:19:54 +0800 Subject: [PATCH 1/5] test: pylib: specify experimental_features explicitly "experimental" was marked deprecated in 8b917f7c. so let's specify the experimental features explicitly using `experimental_feature` option. Signed-off-by: Kefu Chai --- test/pylib/scylla_cluster.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/pylib/scylla_cluster.py b/test/pylib/scylla_cluster.py index a91abb1302..dd7cd5ea23 100644 --- a/test/pylib/scylla_cluster.py +++ b/test/pylib/scylla_cluster.py @@ -71,8 +71,12 @@ def make_scylla_conf(workdir: pathlib.Path, host_addr: str, seed_addrs: List[str # Allow testing experimental features. Following issue #9467, we need # to add here specific experimental features as they are introduced. 'enable_user_defined_functions': True, - 'experimental': True, - 'experimental_features': ['udf', 'consistent-topology-changes'], + 'experimental_features': ['udf', + 'alternator-streams', + 'consistent-topology-changes', + 'broadcast-tables', + 'keyspace-storage-options', + 'tablets'], 'consistent_cluster_management': True, From 959362f85b3cc164c6aa7d39a168d32328f7eda6 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Tue, 8 Aug 2023 16:58:58 +0800 Subject: [PATCH 2/5] test: disable 'enable_user_defined_functions' if experimental_features does not include udf "enable_user_defined_functions" is enabled by default by `make_scylla_conf()` in pylib/scylla_cluster.py, and we've being using `experimental` = True in this very function. this combination works fine, as "udf" is enabled by `experimental`. but since `experimental` is deprecated, if we drop this option and stop handling it. this peace is broken. "enable_user_defined_function" requires "udf" experimental feature. but test_boost_after_ip_change feed the scylla with an empty `experimental_features` list, so the test fails. to pave for the road of dropping `experimental` option, let's disable `enable_user_defined_function` as well in test_boost_after_ip_change. the same applies to other tests changed in this commit. Signed-off-by: Kefu Chai --- test/broadcast_tables/suite.yaml | 5 ++++- test/topology_custom/test_boot_after_ip_change.py | 3 ++- test/topology_custom/test_replace_ignore_nodes.py | 3 ++- test/topology_custom/test_topology_remove_garbage_group0.py | 3 ++- test/topology_experimental_raft/suite.yaml | 1 + test/topology_raft_disabled/suite.yaml | 1 + 6 files changed, 12 insertions(+), 4 deletions(-) diff --git a/test/broadcast_tables/suite.yaml b/test/broadcast_tables/suite.yaml index f5df721409..822c3bfb71 100644 --- a/test/broadcast_tables/suite.yaml +++ b/test/broadcast_tables/suite.yaml @@ -1,2 +1,5 @@ type: Python -extra_scylla_cmdline_options: ["--consistent-cluster-management=true", "--experimental-features=broadcast-tables"] +extra_scylla_cmdline_options: + - "--consistent-cluster-management=true" + - "--experimental-features=broadcast-tables" + - "--enable-user-defined-functions=false" diff --git a/test/topology_custom/test_boot_after_ip_change.py b/test/topology_custom/test_boot_after_ip_change.py index e3f3d07d5d..eb7ca59969 100644 --- a/test/topology_custom/test_boot_after_ip_change.py +++ b/test/topology_custom/test_boot_after_ip_change.py @@ -21,7 +21,8 @@ async def test_boot_after_ip_change(manager: ManagerClient) -> None: """Bootstrap a new node after existing one changed its IP. Regression test for #14468. Does not apply to Raft-topology mode. """ - cfg = {'experimental_features': list[str]()} + cfg = {'enable_user_defined_functions': False, + 'experimental_features': list[str]()} logger.info(f"Booting initial cluster") servers = [await manager.server_add(config=cfg) for _ in range(2)] await wait_for_token_ring_and_group0_consistency(manager, time.time() + 30) diff --git a/test/topology_custom/test_replace_ignore_nodes.py b/test/topology_custom/test_replace_ignore_nodes.py index c91278648d..b7ed9e350a 100644 --- a/test/topology_custom/test_replace_ignore_nodes.py +++ b/test/topology_custom/test_replace_ignore_nodes.py @@ -25,7 +25,8 @@ async def test_replace_ignore_nodes(manager: ManagerClient) -> None: we don't want to run it in debug mode. Preferably run it only in one mode e.g. dev. """ - cfg = {'experimental_features': list[str]()} + cfg = {'enable_user_defined_functions': False, + 'experimental_features': list[str]()} logger.info(f"Booting initial cluster") servers = [await manager.server_add(config=cfg) for _ in range(7)] s2_id = await manager.get_host_id(servers[2].server_id) diff --git a/test/topology_custom/test_topology_remove_garbage_group0.py b/test/topology_custom/test_topology_remove_garbage_group0.py index 2995a2c453..eec82d6f9f 100644 --- a/test/topology_custom/test_topology_remove_garbage_group0.py +++ b/test/topology_custom/test_topology_remove_garbage_group0.py @@ -24,7 +24,8 @@ async def test_remove_garbage_group0_members(manager: ManagerClient): even though the node is no longer a token ring member. Does not apply to Raft-topology mode. """ # 4 servers, one dead - cfg = {'experimental_features': list[str]()} + cfg = {'enable_user_defined_functions': False, + 'experimental_features': list[str]()} servers = [await manager.server_add(config=cfg) for _ in range(4)] removed_host_id = await manager.get_host_id(servers[0].server_id) await manager.server_stop_gracefully(servers[0].server_id) diff --git a/test/topology_experimental_raft/suite.yaml b/test/topology_experimental_raft/suite.yaml index b93ba687a6..ca5d03adce 100644 --- a/test/topology_experimental_raft/suite.yaml +++ b/test/topology_experimental_raft/suite.yaml @@ -5,6 +5,7 @@ cluster: extra_scylla_config_options: authenticator: AllowAllAuthenticator authorizer: AllowAllAuthorizer + enable_user_defined_functions: False experimental_features: ['consistent-topology-changes', 'tablets'] skip_in_release: - test_blocked_bootstrap diff --git a/test/topology_raft_disabled/suite.yaml b/test/topology_raft_disabled/suite.yaml index d78f6043fb..8a8f8189f9 100644 --- a/test/topology_raft_disabled/suite.yaml +++ b/test/topology_raft_disabled/suite.yaml @@ -5,6 +5,7 @@ cluster: extra_scylla_config_options: authenticator: AllowAllAuthenticator authorizer: AllowAllAuthorizer + enable_user_defined_functions: False experimental_features: [] consistent_cluster_management: False run_first: From 64bc8d2f7d0093b6c586ff7f1aff6a2ae59b59ac Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Tue, 8 Aug 2023 09:55:43 +0800 Subject: [PATCH 3/5] config: drop "experimental" option "experimental" was marked deprecated in 8b917f7c. this change was included since Scylla 4.6. now that 5.3 has been branched, this change will be included 5.4. this should be long enough for the user's turn around if this option is ever used. the dtests using this option has been audited and updated accordingly. and the unit testing this option is removed as well. Signed-off-by: Kefu Chai --- db/config.cc | 9 +-------- db/config.hh | 2 -- test/boost/config_test.cc | 12 ------------ tools/schema_loader.cc | 1 - 4 files changed, 1 insertion(+), 23 deletions(-) diff --git a/db/config.cc b/db/config.cc index 5dc5cd0427..f0cdd3e9ef 100644 --- a/db/config.cc +++ b/db/config.cc @@ -861,7 +861,7 @@ db::config::config(std::shared_ptr exts) , developer_mode(this, "developer_mode", value_status::Used, DEVELOPER_MODE_DEFAULT, "Relax environment checks. Setting to true can reduce performance and reliability significantly.") , skip_wait_for_gossip_to_settle(this, "skip_wait_for_gossip_to_settle", value_status::Used, -1, "An integer to configure the wait for gossip to settle. -1: wait normally, 0: do not wait at all, n: wait for at most n polls. Same as -Dcassandra.skip_wait_for_gossip_to_settle in cassandra.") , force_gossip_generation(this, "force_gossip_generation", liveness::LiveUpdate, value_status::Used, -1 , "Force gossip to use the generation number provided by user") - , experimental(this, "experimental", value_status::Used, false, "[Deprecated] Set to true to unlock all experimental features (except 'consistent-topology-changes' feature, which should be enabled explicitly via 'experimental-features' option). Please use 'experimental-features', instead.") + , experimental(this, "experimental", value_status::Unused, false, "[Deprecated] Set to true to unlock all experimental features (except 'consistent-topology-changes' feature, which should be enabled explicitly via 'experimental-features' option). Please use 'experimental-features', instead.") , experimental_features(this, "experimental_features", value_status::Used, {}, experimental_features_help_string()) , lsa_reclamation_step(this, "lsa_reclamation_step", value_status::Used, 1, "Minimum number of segments to reclaim in a single step") , prometheus_port(this, "prometheus_port", value_status::Used, 9180, "Prometheus port, set to zero to disable") @@ -1147,13 +1147,6 @@ 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::feature::UNUSED - && f != experimental_features_t::feature::CONSISTENT_TOPOLOGY_CHANGES - && f != experimental_features_t::feature::BROADCAST_TABLES - && f != experimental_features_t::feature::TABLETS) { - return true; - } const auto& optval = experimental_features(); return find(begin(optval), end(optval), enum_option{f}) != end(optval); } diff --git a/db/config.hh b/db/config.hh index d73e13581c..553bba1b1b 100644 --- a/db/config.hh +++ b/db/config.hh @@ -97,8 +97,6 @@ namespace db { /// Enumeration of all valid values for the `experimental` config entry. struct experimental_features_t { - // NOTE: CONSISTENT_TOPOLOGY_CHANGES and BROADCAST_TABLES features are not enabled via `experimental` umbrella flag. - // These options should be enabled explicitly. enum class feature { UNUSED, UDF, diff --git a/test/boost/config_test.cc b/test/boost/config_test.cc index d88a46312c..5549ad1912 100644 --- a/test/boost/config_test.cc +++ b/test/boost/config_test.cc @@ -1035,18 +1035,6 @@ SEASTAR_TEST_CASE(test_parse_experimental_features_invalid) { return make_ready_future(); } -SEASTAR_TEST_CASE(test_parse_experimental_true) { - auto cfg_ptr = std::make_unique(); - config& cfg = *cfg_ptr; - cfg.read_from_yaml("experimental: true", throw_on_error); - BOOST_CHECK(!cfg.check_experimental(ef::UNUSED)); - BOOST_CHECK(cfg.check_experimental(ef::UDF)); - BOOST_CHECK(cfg.check_experimental(ef::ALTERNATOR_STREAMS)); - BOOST_CHECK(!cfg.check_experimental(ef::CONSISTENT_TOPOLOGY_CHANGES)); - BOOST_CHECK(cfg.check_experimental(ef::KEYSPACE_STORAGE_OPTIONS)); - return make_ready_future(); -} - SEASTAR_TEST_CASE(test_parse_experimental_false) { auto cfg_ptr = std::make_unique(); config& cfg = *cfg_ptr; diff --git a/tools/schema_loader.cc b/tools/schema_loader.cc index 3ca25f53c6..208025404e 100644 --- a/tools/schema_loader.cc +++ b/tools/schema_loader.cc @@ -564,7 +564,6 @@ future load_one_schema_from_file(std::filesystem::path path) { schema_ptr load_system_schema(std::string_view keyspace, std::string_view table) { db::config cfg; - cfg.experimental.set(true); cfg.experimental_features.set(db::experimental_features_t::all()); const std::unordered_map> schemas{ {db::schema_tables::NAME, db::schema_tables::all_tables(db::schema_features::full())}, From 6355270120dd195bcd7768021c483d4153085a8f Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Tue, 8 Aug 2023 10:02:08 +0800 Subject: [PATCH 4/5] config: use std::ranges when appropriate use std::ranges functions for better readability. Signed-off-by: Kefu Chai --- db/config.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/db/config.cc b/db/config.cc index f0cdd3e9ef..e332e17fca 100644 --- a/db/config.cc +++ b/db/config.cc @@ -1147,8 +1147,11 @@ db::fs::path db::config::get_conf_sub(db::fs::path sub) { } bool db::config::check_experimental(experimental_features_t::feature f) const { - const auto& optval = experimental_features(); - return find(begin(optval), end(optval), enum_option{f}) != end(optval); + enum_option to_check{f}; + return std::ranges::any_of(experimental_features(), + [to_check](auto& enabled) { + return to_check == enabled; + }); } namespace bpo = boost::program_options; From 153a808f52222f8927c6d7b2941d776233851b36 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Tue, 8 Aug 2023 10:02:32 +0800 Subject: [PATCH 5/5] config: remove unused namespace alias bpo is not used after it is defined, so drop it. Signed-off-by: Kefu Chai --- db/config.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/db/config.cc b/db/config.cc index e332e17fca..4d6437dbb9 100644 --- a/db/config.cc +++ b/db/config.cc @@ -1154,8 +1154,6 @@ bool db::config::check_experimental(experimental_features_t::feature f) const { }); } -namespace bpo = boost::program_options; - logging::settings db::config::logging_settings(const log_cli::options& opts) const { auto value = [&](auto& v, const auto& opt) { if (opt.defaulted() && v.is_set()) {