Skip to content

Commit

Permalink
storage_proxy: query_partition_key_range_concurrent: don't access emp…
Browse files Browse the repository at this point in the history
…ty range

`query_partition_range_concurrent` implements an optimization when
querying a token range that intersects multiple vnodes. Instead of
sending a query for each vnode separately, it sometimes sends a single
query to cover multiple vnodes - if the intersection of replica sets for
those vnodes is large enough to satisfy the CL and good enough in terms
of the heat metric. To check the latter condition, the code would take
the smallest heat metric of the intersected replica set and compare them
to smallest heat metrics of replica sets calculated separately for each
vnode.

Unfortunately, there was an edge case that the code didn't handle: the
intersected replica set might be empty and the code would access an
empty range.

This was catched by an assertion added in
8db1d75 by the dtest
`test_query_dc_with_rf_0_does_not_crash_db`.

The fix is simple: check if the intersected set is empty - if so, don't
calculate the heat metrics because we can decide early that the
optimization doesn't apply.

Also change the `assert` to `on_internal_error`.

Fixes #14284

Closes #14300
  • Loading branch information
kbr-scylla authored and denesb committed Jun 20, 2023
1 parent ddf8547 commit 732feca
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions service/storage_proxy.cc
Expand Up @@ -5586,7 +5586,9 @@ storage_proxy::query_partition_key_range_concurrent(storage_proxy::clock_type::t
inet_address_vector_replica_set filtered_merged = filter_replicas_for_read(cl, *erm, merged, current_merged_preferred_replicas, pcf);

// Estimate whether merging will be a win or not
if (!is_worth_merging_for_range_query(erm->get_topology(), filtered_merged, filtered_endpoints, next_filtered_endpoints)) {
if (filtered_merged.empty()
|| !is_worth_merging_for_range_query(
erm->get_topology(), filtered_merged, filtered_endpoints, next_filtered_endpoints)) {
break;
} else if (pcf) {
// check that merged set hit rate is not to low
Expand All @@ -5606,7 +5608,9 @@ storage_proxy::query_partition_key_range_concurrent(storage_proxy::clock_type::t
}
} ep_to_hr{remote().gossiper(), pcf};

assert (!range.empty());
if (range.empty()) {
on_internal_error(slogger, "empty range passed to `find_min`");
}
return *boost::range::min_element(range | boost::adaptors::transformed(ep_to_hr));
};
auto merged = find_min(filtered_merged) * 1.2; // give merged set 20% boost
Expand Down

0 comments on commit 732feca

Please sign in to comment.