Skip to content

Commit

Permalink
query_processor: make execute_internal caching parameter more verbose
Browse files Browse the repository at this point in the history 
`execute_internal` has a parameter to indicate if caching a prepared
statement is needed for a specific call. However this parameter was a
boolean so it was easy to miss it's meaning in the various call sites.
This replaces the parameter type to a more verbose one so it is clear
from the call site what decision was made.
  • Loading branch information
Eliran Sinvani committed May 1, 2022
1 parent 7f1e368 commit 38b7ebf
Show file tree
Hide file tree
Showing 9 changed files with 28 additions and 26 deletions.
4 changes: 2 additions & 2 deletions auth/default_authorizer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ future<bool> default_authorizer::any_granted() const {
query,
db::consistency_level::LOCAL_ONE,
{},
true).then([this](::shared_ptr<cql3::untyped_result_set> results) {
cql3::query_processor::cache_internal::yes).then([this](::shared_ptr<cql3::untyped_result_set> results) {
return !results->empty();
});
}
Expand Down Expand Up @@ -223,7 +223,7 @@ future<std::vector<permission_details>> default_authorizer::list_all() const {
db::consistency_level::ONE,
internal_distributed_query_state(),
{},
true).then([](::shared_ptr<cql3::untyped_result_set> results) {
cql3::query_processor::cache_internal::yes).then([](::shared_ptr<cql3::untyped_result_set> results) {
std::vector<permission_details> all_details;

for (const auto& row : *results) {
Expand Down
2 changes: 1 addition & 1 deletion auth/password_authenticator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ future<authenticated_user> password_authenticator::authenticate(
consistency_for_user(username),
internal_distributed_query_state(),
{username},
true);
cql3::query_processor::cache_internal::yes);
}).then_wrapped([=](future<::shared_ptr<cql3::untyped_result_set>> f) {
try {
auto res = f.get0();
Expand Down
4 changes: 2 additions & 2 deletions auth/roles-metadata.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,14 @@ future<bool> default_role_row_satisfies(
query,
db::consistency_level::ONE,
{meta::DEFAULT_SUPERUSER_NAME},
true).then([&qp, &p](::shared_ptr<cql3::untyped_result_set> results) {
cql3::query_processor::cache_internal::yes).then([&qp, &p](::shared_ptr<cql3::untyped_result_set> results) {
if (results->empty()) {
return qp.execute_internal(
query,
db::consistency_level::QUORUM,
internal_distributed_query_state(),
{meta::DEFAULT_SUPERUSER_NAME},
true).then([&p](::shared_ptr<cql3::untyped_result_set> results) {
cql3::query_processor::cache_internal::yes).then([&p](::shared_ptr<cql3::untyped_result_set> results) {
if (results->empty()) {
return make_ready_future<bool>(false);
}
Expand Down
4 changes: 2 additions & 2 deletions auth/service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ future<bool> service::has_existing_legacy_users() const {
default_user_query,
db::consistency_level::ONE,
{meta::DEFAULT_SUPERUSER_NAME},
true).then([this](auto results) {
cql3::query_processor::cache_internal::yes).then([this](auto results) {
if (!results->empty()) {
return make_ready_future<bool>(true);
}
Expand All @@ -212,7 +212,7 @@ future<bool> service::has_existing_legacy_users() const {
default_user_query,
db::consistency_level::QUORUM,
{meta::DEFAULT_SUPERUSER_NAME},
true).then([this](auto results) {
cql3::query_processor::cache_internal::yes).then([this](auto results) {
if (!results->empty()) {
return make_ready_future<bool>(true);
}
Expand Down
4 changes: 2 additions & 2 deletions auth/standard_role_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ static future<std::optional<record>> find_record(cql3::query_processor& qp, std:
consistency_for_role(role_name),
internal_distributed_query_state(),
{sstring(role_name)},
true).then([](::shared_ptr<cql3::untyped_result_set> results) {
cql3::query_processor::cache_internal::yes).then([](::shared_ptr<cql3::untyped_result_set> results) {
if (results->empty()) {
return std::optional<record>();
}
Expand Down Expand Up @@ -267,7 +267,7 @@ future<> standard_role_manager::create_or_replace(std::string_view role_name, co
consistency_for_role(role_name),
internal_distributed_query_state(),
{sstring(role_name), c.is_superuser, c.can_login},
true).discard_result();
cql3::query_processor::cache_internal::yes).discard_result();
}

future<>
Expand Down
4 changes: 2 additions & 2 deletions cql3/query_processor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ query_processor::execute_internal(
const sstring& query_string,
db::consistency_level cl,
const std::initializer_list<data_value>& values,
bool cache) {
cache_internal cache) {
return execute_internal(query_string, cl, *_internal_state, values, cache);
}

Expand All @@ -818,7 +818,7 @@ query_processor::execute_internal(
db::consistency_level cl,
service::query_state& query_state,
const std::initializer_list<data_value>& values,
bool cache) {
cache_internal cache) {

if (log.is_enabled(logging::log_level::trace)) {
log.trace("execute_internal: {}\"{}\" ({})", cache ? "(cached) " : "", query_string, ::join(", ", values));
Expand Down
8 changes: 5 additions & 3 deletions cql3/query_processor.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public:
// creating namespaces, etc) is explicitly forbidden via this interface.
future<::shared_ptr<untyped_result_set>>
execute_internal(const sstring& query_string, const std::initializer_list<data_value>& values = { }) {
return execute_internal(query_string, db::consistency_level::ONE, values, true);
return execute_internal(query_string, db::consistency_level::ONE, values, cache_internal::yes);
}

statements::prepared_statement::checked_weak_ptr prepare_internal(const sstring& query);
Expand Down Expand Up @@ -298,6 +298,8 @@ public:
const sstring& query_string,
noncopyable_function<future<stop_iteration>(const cql3::untyped_result_set_row&)>&& f);

class cache_internal_tag;
using cache_internal = bool_class<cache_internal_tag>;
// NOTICE: Internal queries should be used with care, as they are expected
// to be used for local tables (e.g. from the `system` keyspace).
// Data modifications will usually be performed with consistency level ONE
Expand All @@ -308,13 +310,13 @@ public:
const sstring& query_string,
db::consistency_level,
const std::initializer_list<data_value>& = { },
bool cache = false);
cache_internal cache = cache_internal::no);
future<::shared_ptr<untyped_result_set>> execute_internal(
const sstring& query_string,
db::consistency_level,
service::query_state& query_state,
const std::initializer_list<data_value>& = { },
bool cache = false);
cache_internal cache = cache_internal::no);

future<::shared_ptr<untyped_result_set>> execute_with_params(
statements::prepared_statement::checked_weak_ptr p,
Expand Down
2 changes: 1 addition & 1 deletion db/query_context.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ struct query_context {
cql3::query_options::DEFAULT.get_consistency(),
tctx.query_state,
{ data_value(std::forward<Args>(args))... },
true);
cql3::query_processor::cache_internal::yes);
});
}

Expand Down
22 changes: 11 additions & 11 deletions db/system_distributed_keyspace.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ future<std::unordered_map<utils::UUID, sstring>> system_distributed_keyspace::vi
db::consistency_level::ONE,
internal_distributed_query_state(),
{ std::move(ks_name), std::move(view_name) },
false).then([this] (::shared_ptr<cql3::untyped_result_set> cql_result) {
cql3::query_processor::cache_internal::no).then([this] (::shared_ptr<cql3::untyped_result_set> cql_result) {
return boost::copy_range<std::unordered_map<utils::UUID, sstring>>(*cql_result
| boost::adaptors::transformed([] (const cql3::untyped_result_set::row& row) {
auto host_id = row.get_as<utils::UUID>("host_id");
Expand All @@ -366,7 +366,7 @@ future<> system_distributed_keyspace::start_view_build(sstring ks_name, sstring
db::consistency_level::ONE,
internal_distributed_query_state(),
{ std::move(ks_name), std::move(view_name), std::move(host_id), "STARTED" },
false).discard_result();
cql3::query_processor::cache_internal::no).discard_result();
}

future<> system_distributed_keyspace::finish_view_build(sstring ks_name, sstring view_name) const {
Expand All @@ -376,7 +376,7 @@ future<> system_distributed_keyspace::finish_view_build(sstring ks_name, sstring
db::consistency_level::ONE,
internal_distributed_query_state(),
{ "SUCCESS", std::move(ks_name), std::move(view_name), std::move(host_id) },
false).discard_result();
cql3::query_processor::cache_internal::no).discard_result();
}

future<> system_distributed_keyspace::remove_view(sstring ks_name, sstring view_name) const {
Expand All @@ -385,7 +385,7 @@ future<> system_distributed_keyspace::remove_view(sstring ks_name, sstring view_
db::consistency_level::ONE,
internal_distributed_query_state(),
{ std::move(ks_name), std::move(view_name) },
false).discard_result();
cql3::query_processor::cache_internal::no).discard_result();
}

/* We want to make sure that writes/reads to/from CDC management-related distributed tables
Expand Down Expand Up @@ -458,7 +458,7 @@ system_distributed_keyspace::insert_cdc_topology_description(
quorum_if_many(ctx.num_token_owners),
internal_distributed_query_state(),
{ gen_id.ts, make_list_value(cdc_generation_description_type, prepare_cdc_generation_description(description)) },
false).discard_result();
cql3::query_processor::cache_internal::no).discard_result();
}

future<std::optional<cdc::topology_description>>
Expand All @@ -471,7 +471,7 @@ system_distributed_keyspace::read_cdc_topology_description(
quorum_if_many(ctx.num_token_owners),
internal_distributed_query_state(),
{ gen_id.ts },
false
cql3::query_processor::cache_internal::no
).then([] (::shared_ptr<cql3::untyped_result_set> cql_result) -> std::optional<cdc::topology_description> {
if (cql_result->empty() || !cql_result->one().has("description")) {
return {};
Expand Down Expand Up @@ -669,7 +669,7 @@ system_distributed_keyspace::create_cdc_desc(
quorum_if_many(ctx.num_token_owners),
internal_distributed_query_state(),
{ CDC_TIMESTAMPS_KEY, time },
false).discard_result();
cql3::query_processor::cache_internal::no).discard_result();
}

future<bool>
Expand Down Expand Up @@ -714,7 +714,7 @@ system_distributed_keyspace::cdc_desc_exists(
quorum_if_many(ctx.num_token_owners),
internal_distributed_query_state(),
{ CDC_TIMESTAMPS_KEY, streams_ts },
false
cql3::query_processor::cache_internal::no
).then([] (::shared_ptr<cql3::untyped_result_set> cql_result) -> bool {
return !cql_result->empty() && cql_result->one().has("time");
});
Expand All @@ -727,7 +727,7 @@ system_distributed_keyspace::cdc_get_versioned_streams(db_clock::time_point not_
quorum_if_many(ctx.num_token_owners),
internal_distributed_query_state(),
{ CDC_TIMESTAMPS_KEY },
false);
cql3::query_processor::cache_internal::no);

std::vector<db_clock::time_point> timestamps;
timestamps.reserve(timestamps_cql->size());
Expand All @@ -749,7 +749,7 @@ system_distributed_keyspace::cdc_get_versioned_streams(db_clock::time_point not_
quorum_if_many(ctx.num_token_owners),
internal_distributed_query_state(),
{ ts },
false);
cql3::query_processor::cache_internal::no);

utils::chunked_vector<cdc::stream_id> ids;
for (auto& row : *streams_cql) {
Expand All @@ -770,7 +770,7 @@ system_distributed_keyspace::cdc_current_generation_timestamp(context ctx) {
quorum_if_many(ctx.num_token_owners),
internal_distributed_query_state(),
{ CDC_TIMESTAMPS_KEY },
false);
cql3::query_processor::cache_internal::no);

co_return timestamp_cql->one().get_as<db_clock::time_point>("time");
}
Expand Down

0 comments on commit 38b7ebf

Please sign in to comment.