Skip to content

Commit

Permalink
query_ranges_to_vnodes_generator: fix for exclusive boundaries
Browse files Browse the repository at this point in the history
Let the initial range passed to query_partition_key_range
be [1, 2) where 2 is the successor of 1 in terms
of ring_position order and 1 is equal to vnode.
Then query_ranges_to_vnodes_generator() -> [[1, 1], (1, 2)],
so we get an empty range (1,2) and subsequently will
make a data request with this empty range in
storage_proxy::query_partition_key_range_concurrent,
which will be redundant.

The patch checks for this condition after the main loop
in process_one_range.

A test case is added for this scenario in
test_get_restricted_ranges. The helper
lambda check is changed so that not to limit
the number of ranges to the length of expected
ranges, otherwise this check passes without
the change in process_one_range.

Fixes: scylladb#12566
  • Loading branch information
Petr Gusev committed Feb 6, 2023
1 parent 5d914ad commit 46f67e9
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 2 deletions.
9 changes: 8 additions & 1 deletion query_ranges_to_vnodes.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ void query_ranges_to_vnodes_generator::process_one_range(size_t n, dht::partitio
}

if (ranges.size() < n) {
add_range(get_remainder());
auto range = get_remainder();
const bool is_empty = range.start().has_value() && range.end().has_value() &&
range.start()->value().relation_to_keys() - range.end()->value().relation_to_keys() == 2 &&
dht::token::to_int64(range.end()->value().token()) - dht::token::to_int64(range.start()->value().token()) == 1 &&
(!range.start()->is_inclusive() || !range.end()->is_inclusive());
if (!is_empty) {
add_range(std::move(range));
}
}
}
22 changes: 21 additions & 1 deletion test/boost/storage_proxy_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,34 @@ SEASTAR_TEST_CASE(test_get_restricted_ranges) {
auto check = [&s](locator::token_metadata_ptr tmptr, dht::partition_range input,
dht::partition_range_vector expected) {
query_ranges_to_vnodes_generator ranges_to_vnodes(tmptr, s, {input});
auto actual = ranges_to_vnodes(expected.size());
auto actual = ranges_to_vnodes(1000);
if (!std::equal(actual.begin(), actual.end(), expected.begin(), [&s](auto&& r1, auto&& r2) {
return r1.equal(r2, dht::ring_position_comparator(*s));
})) {
BOOST_FAIL(format("Ranges differ, expected {} but got {}", expected, actual));
}
};

{
const auto endpoint = gms::inet_address("10.0.0.1");
const auto endpoint_token = ring[2].token();
auto tmptr = locator::make_token_metadata_ptr(locator::token_metadata::config{});
tmptr->update_topology(endpoint, {});
tmptr->update_normal_tokens({endpoint_token}, endpoint).get();

const auto next_token = dht::token::from_int64(dht::token::to_int64(endpoint_token) + 1);
const auto endpoint_token_ending_bound = dht::partition_range::bound {
dht::ring_position::ending_at(endpoint_token), true
};
const auto input = dht::partition_range::make(
endpoint_token_ending_bound,
{dht::ring_position::starting_at(next_token), false});
const auto expected_output = dht::partition_range::make(
endpoint_token_ending_bound,
endpoint_token_ending_bound);
check(tmptr, input, { expected_output });
}

{
// Ring with minimum token
auto tmptr = locator::make_token_metadata_ptr(locator::token_metadata::config{});
Expand Down

0 comments on commit 46f67e9

Please sign in to comment.