[PATCH] main: Replace cql_config_updater with updateable_value

[Pavel Emelyanov]

The cql_config_updater is a sharded<> service that exists in main and
whose goal is to make sure some db::config's values are propagated into
cql_config. There's a more handy updateable_value<> glue for that.

tests: unit(dev)
refs: #2795

Signed-off-by: Pavel Emelyanov <xe...@scylladb.com>
---
cql3/cql_config.hh | 5 +++++
cql3/restrictions/restrictions_config.hh | 17 ++++++++++++++--
cql3/query_options.cc | 4 ++--
main.cc | 25 +-----------------------
test/boost/restrictions_test.cc | 2 +-
test/lib/cql_test_env.cc | 2 +-
6 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/cql3/cql_config.hh b/cql3/cql_config.hh
index 3e5dd12d09..1b81db4c15 100644
--- a/cql3/cql_config.hh
+++ b/cql3/cql_config.hh
@@ -25,10 +25,15 @@

#include "restrictions/restrictions_config.hh"

+namespace db { class config; }
+
namespace cql3 {

struct cql_config {
restrictions::restrictions_config restrictions;
+ explicit cql_config(const db::config& cfg) : restrictions(cfg) {}
+ struct default_tag{};
+ cql_config(default_tag) : restrictions(restrictions::restrictions_config::default_tag{}) {}
};

extern const cql_config default_cql_config;
diff --git a/cql3/restrictions/restrictions_config.hh b/cql3/restrictions/restrictions_config.hh
index 8df08b749b..0bfb807005 100644
--- a/cql3/restrictions/restrictions_config.hh
+++ b/cql3/restrictions/restrictions_config.hh
@@ -24,12 +24,25 @@
#pragma once

#include <cstdint>
+#include "db/config.hh"
+#include "utils/updateable_value.hh"

namespace cql3::restrictions {

struct restrictions_config {
- uint32_t partition_key_restrictions_max_cartesian_product_size = 100;
- uint32_t clustering_key_restrictions_max_cartesian_product_size = 100;
+ utils::updateable_value<uint32_t> partition_key_restrictions_max_cartesian_product_size;
+ utils::updateable_value<uint32_t> clustering_key_restrictions_max_cartesian_product_size;
+
+ explicit restrictions_config(const db::config& cfg)
+ : partition_key_restrictions_max_cartesian_product_size(cfg.max_partition_key_restrictions_per_query)
+ , clustering_key_restrictions_max_cartesian_product_size(cfg.max_clustering_key_restrictions_per_query)
+ {}
+
+ struct default_tag{};
+ restrictions_config(default_tag)
+ : partition_key_restrictions_max_cartesian_product_size(100)
+ , clustering_key_restrictions_max_cartesian_product_size(100)
+ {}
};

}
diff --git a/cql3/query_options.cc b/cql3/query_options.cc
index b518db691c..2d88993e3e 100644
--- a/cql3/query_options.cc
+++ b/cql3/query_options.cc
@@ -47,7 +47,7 @@

namespace cql3 {

-const cql_config default_cql_config;
+const cql_config default_cql_config(cql_config::default_tag{});

thread_local const query_options::specific_options query_options::specific_options::DEFAULT{
-1, {}, db::consistency_level::SERIAL, api::missing_timestamp};
@@ -215,4 +215,4 @@ computed_function_values::mapped_type* query_options::find_cached_pk_function_ca
return nullptr;
}

-}
\ No newline at end of file
+}
diff --git a/main.cc b/main.cc
index cadccc30be..a3b239e226 100644
--- a/main.cc
+++ b/main.cc
@@ -367,25 +367,6 @@ void print_starting_message(int ac, char** av, const bpo::parsed_options& opts)
fmt::print("parsed command line options: {}\n", format_parsed_options(opts.options));
}

-// Glue logic between db::config and cql3::cql_config
-class cql_config_updater {
- cql3::cql_config& _cql_config;
- const db::config& _cfg;
- std::vector<std::any> _observers;
-private:
- template <typename T>
- void tie(T& dest, const db::config::named_value<T>& src) {
- dest = src();
- _observers.emplace_back(make_lw_shared<utils::observer<T>>(src.observe([&dest] (const T& value) { dest = value; })));
- }
-public:
- cql_config_updater(cql3::cql_config& cql_config, const db::config& cfg)
- : _cql_config(cql_config), _cfg(cfg) {
- tie(_cql_config.restrictions.partition_key_restrictions_max_cartesian_product_size, _cfg.max_partition_key_restrictions_per_query);
- tie(_cql_config.restrictions.clustering_key_restrictions_max_cartesian_product_size, _cfg.max_clustering_key_restrictions_per_query);
- }
-};
-
template <typename Func>
static auto defer_verbose_shutdown(const char* what, Func&& func) {
auto vfunc = [what, func = std::forward<Func>(func)] () mutable {
@@ -830,12 +811,8 @@ int main(int ac, char** av) {
static sharded<db::system_distributed_keyspace> sys_dist_ks;
static sharded<db::view::view_update_generator> view_update_generator;
static sharded<cql3::cql_config> cql_config;
- static sharded<::cql_config_updater> cql_config_updater;
static sharded<cdc::generation_service> cdc_generation_service;
- cql_config.start().get();
- //FIXME: discarded future
- (void)cql_config_updater.start(std::ref(cql_config), std::ref(*cfg));
- auto stop_cql_config_updater = deferred_stop(cql_config_updater);
+ cql_config.start(std::ref(*cfg)).get();

supervisor::notify("starting gossiper");
gms::gossip_config gcfg;
diff --git a/test/boost/restrictions_test.cc b/test/boost/restrictions_test.cc
index 72cb70ccbc..31ca790016 100644
--- a/test/boost/restrictions_test.cc
+++ b/test/boost/restrictions_test.cc
@@ -60,7 +60,7 @@ void require_rows(cql_test_env& e,
const std::experimental::source_location& loc = source_location::current()) {
// This helps compiler pick the right overload for make_value:
const auto rvals = values | transformed([] (const bytes_opt& v) { return cql3::raw_value::make_value(v); });
- cql3::cql_config cfg;
+ cql3::cql_config cfg(cql3::cql_config::default_tag{});
auto opts = to_options(cfg, std::move(names), std::vector(rvals.begin(), rvals.end()));
try {
assert_that(e.execute_prepared_with_qo(id, std::move(opts)).get0()).is_rows().with_rows_ignore_order(expected);
diff --git a/test/lib/cql_test_env.cc b/test/lib/cql_test_env.cc
index 2340abeb4e..7f97d5dea6 100644
--- a/test/lib/cql_test_env.cc
+++ b/test/lib/cql_test_env.cc
@@ -556,7 +556,7 @@ class single_node_cql_env : public cql_test_env {
distributed<service::migration_manager> mm;
distributed<db::batchlog_manager>& bm = db::get_batchlog_manager();
sharded<cql3::cql_config> cql_config;
- cql_config.start().get();
+ cql_config.start(cql3::cql_config::default_tag{}).get();
auto stop_cql_config = defer([&] { cql_config.stop().get(); });

sharded<db::view::view_update_generator> view_update_generator;
--
2.20.1

[Commit Bot]

From: Pavel Emelyanov <xe...@scylladb.com>
Committer: Botond Dénes <bde...@scylladb.com>
Branch: next

main: Replace cql_config_updater with updateable_value

The cql_config_updater is a sharded<> service that exists in main and
whose goal is to make sure some db::config's values are propagated into
cql_config. There's a more handy updateable_value<> glue for that.

tests: unit(dev)
refs: #2795

Signed-off-by: Pavel Emelyanov <xe...@scylladb.com>

Message-Id: <20210927090402...@scylladb.com>

---
diff --git a/cql3/cql_config.hh b/cql3/cql_config.hh

--- a/cql3/cql_config.hh
+++ b/cql3/cql_config.hh
@@ -25,10 +25,15 @@

#include "restrictions/restrictions_config.hh"

+namespace db { class config; }
+
namespace cql3 {

struct cql_config {
restrictions::restrictions_config restrictions;
+ explicit cql_config(const db::config& cfg) : restrictions(cfg) {}
+ struct default_tag{};
+ cql_config(default_tag) : restrictions(restrictions::restrictions_config::default_tag{}) {}
};

extern const cql_config default_cql_config;

diff --git a/cql3/query_options.cc b/cql3/query_options.cc

--- a/cql3/query_options.cc
+++ b/cql3/query_options.cc
@@ -47,7 +47,7 @@

namespace cql3 {

-const cql_config default_cql_config;
+const cql_config default_cql_config(cql_config::default_tag{});

thread_local const query_options::specific_options query_options::specific_options::DEFAULT{
-1, {}, db::consistency_level::SERIAL, api::missing_timestamp};
@@ -215,4 +215,4 @@ computed_function_values::mapped_type* query_options::find_cached_pk_function_ca
return nullptr;
}

-}
\ No newline at end of file
+}

diff --git a/cql3/restrictions/restrictions_config.hh b/cql3/restrictions/restrictions_config.hh

--- a/cql3/restrictions/restrictions_config.hh
+++ b/cql3/restrictions/restrictions_config.hh
@@ -24,12 +24,25 @@
#pragma once

#include <cstdint>
+#include "db/config.hh"
+#include "utils/updateable_value.hh"

namespace cql3::restrictions {

struct restrictions_config {
- uint32_t partition_key_restrictions_max_cartesian_product_size = 100;
- uint32_t clustering_key_restrictions_max_cartesian_product_size = 100;
+ utils::updateable_value<uint32_t> partition_key_restrictions_max_cartesian_product_size;
+ utils::updateable_value<uint32_t> clustering_key_restrictions_max_cartesian_product_size;
+
+ explicit restrictions_config(const db::config& cfg)
+ : partition_key_restrictions_max_cartesian_product_size(cfg.max_partition_key_restrictions_per_query)
+ , clustering_key_restrictions_max_cartesian_product_size(cfg.max_clustering_key_restrictions_per_query)
+ {}
+
+ struct default_tag{};
+ restrictions_config(default_tag)
+ : partition_key_restrictions_max_cartesian_product_size(100)
+ , clustering_key_restrictions_max_cartesian_product_size(100)
+ {}
};

}

diff --git a/main.cc b/main.cc

--- a/test/boost/restrictions_test.cc
+++ b/test/boost/restrictions_test.cc
@@ -60,7 +60,7 @@ void require_rows(cql_test_env& e,
const std::experimental::source_location& loc = source_location::current()) {
// This helps compiler pick the right overload for make_value:
const auto rvals = values | transformed([] (const bytes_opt& v) { return cql3::raw_value::make_value(v); });
- cql3::cql_config cfg;
+ cql3::cql_config cfg(cql3::cql_config::default_tag{});
auto opts = to_options(cfg, std::move(names), std::vector(rvals.begin(), rvals.end()));
try {
assert_that(e.execute_prepared_with_qo(id, std::move(opts)).get0()).is_rows().with_rows_ignore_order(expected);
diff --git a/test/lib/cql_test_env.cc b/test/lib/cql_test_env.cc

[Botond Dénes]

AFAIK updateable_value also supports initializing to a constant, so no need to remove the defaults.

[Pavel Emelyanov]

On 30.09.2021 07:24, Botond Dénes wrote:
> On Mon, 2021-09-27 at 12:04 +0300, Pavel Emelyanov wrote:
>> The cql_config_updater is a sharded<> service that exists in main and
>> whose goal is to make sure some db::config's values are propagated into
>> cql_config. There's a more handy updateable_value<> glue for that.
>>
>> tests: unit(dev)
>> refs: #2795
>>

>> Signed-off-by: Pavel Emelyanov <xe...@scylladb.com <mailto:xe...@scylladb.com>>

It does, but the plan is not to have "defatul" constructor, but make callers
pick one of -- from db::config OR the explicitly default one.

[Commit Bot]

From: Pavel Emelyanov <xe...@scylladb.com>
Committer: Botond Dénes <bde...@scylladb.com>

Branch: master

main: Replace cql_config_updater with updateable_value

The cql_config_updater is a sharded<> service that exists in main and
whose goal is to make sure some db::config's values are propagated into
cql_config. There's a more handy updateable_value<> glue for that.

tests: unit(dev)
refs: #2795

Signed-off-by: Pavel Emelyanov <xe...@scylladb.com>

Message-Id: <20210927090402...@scylladb.com>

---
diff --git a/cql3/cql_config.hh b/cql3/cql_config.hh

--- a/cql3/cql_config.hh
+++ b/cql3/cql_config.hh
@@ -25,10 +25,15 @@

#include "restrictions/restrictions_config.hh"

+namespace db { class config; }
+
namespace cql3 {

struct cql_config {
restrictions::restrictions_config restrictions;
+ explicit cql_config(const db::config& cfg) : restrictions(cfg) {}
+ struct default_tag{};
+ cql_config(default_tag) : restrictions(restrictions::restrictions_config::default_tag{}) {}
};

extern const cql_config default_cql_config;

diff --git a/cql3/query_options.cc b/cql3/query_options.cc

--- a/cql3/query_options.cc
+++ b/cql3/query_options.cc
@@ -47,7 +47,7 @@

namespace cql3 {

-const cql_config default_cql_config;
+const cql_config default_cql_config(cql_config::default_tag{});

thread_local const query_options::specific_options query_options::specific_options::DEFAULT{
-1, {}, db::consistency_level::SERIAL, api::missing_timestamp};
@@ -215,4 +215,4 @@ computed_function_values::mapped_type* query_options::find_cached_pk_function_ca
return nullptr;
}

-}
\ No newline at end of file
+}

diff --git a/cql3/restrictions/restrictions_config.hh b/cql3/restrictions/restrictions_config.hh

--- a/cql3/restrictions/restrictions_config.hh
+++ b/cql3/restrictions/restrictions_config.hh
@@ -24,12 +24,25 @@
#pragma once

#include <cstdint>
+#include "db/config.hh"
+#include "utils/updateable_value.hh"

namespace cql3::restrictions {

struct restrictions_config {
- uint32_t partition_key_restrictions_max_cartesian_product_size = 100;
- uint32_t clustering_key_restrictions_max_cartesian_product_size = 100;
+ utils::updateable_value<uint32_t> partition_key_restrictions_max_cartesian_product_size;
+ utils::updateable_value<uint32_t> clustering_key_restrictions_max_cartesian_product_size;
+
+ explicit restrictions_config(const db::config& cfg)
+ : partition_key_restrictions_max_cartesian_product_size(cfg.max_partition_key_restrictions_per_query)
+ , clustering_key_restrictions_max_cartesian_product_size(cfg.max_clustering_key_restrictions_per_query)
+ {}
+
+ struct default_tag{};
+ restrictions_config(default_tag)
+ : partition_key_restrictions_max_cartesian_product_size(100)
+ , clustering_key_restrictions_max_cartesian_product_size(100)
+ {}
};

}

diff --git a/main.cc b/main.cc

--- a/test/boost/restrictions_test.cc
+++ b/test/boost/restrictions_test.cc
@@ -60,7 +60,7 @@ void require_rows(cql_test_env& e,
const std::experimental::source_location& loc = source_location::current()) {
// This helps compiler pick the right overload for make_value:
const auto rvals = values | transformed([] (const bytes_opt& v) { return cql3::raw_value::make_value(v); });
- cql3::cql_config cfg;
+ cql3::cql_config cfg(cql3::cql_config::default_tag{});
auto opts = to_options(cfg, std::move(names), std::vector(rvals.begin(), rvals.end()));
try {
assert_that(e.execute_prepared_with_qo(id, std::move(opts)).get0()).is_rows().with_rows_ignore_order(expected);
diff --git a/test/lib/cql_test_env.cc b/test/lib/cql_test_env.cc

[Avi Kivity]

Will this not run into https://github.com/scylladb/scylla/issues/7316?

[Pavel Emelyanov]

On 30.09.2021 12:17, Avi Kivity wrote:
> Will this not run into https://github.com/scylladb/scylla/issues/7316?

Shouldn't. I checked it with a stupid boost test, but it had to hack around
the db::config const-ness in cql_test_env so I didn't include it here.