From 3842bf18a7cfb09d43209702ebed6414f098ea0e Mon Sep 17 00:00:00 2001 From: Patryk Wrobel Date: Fri, 16 Feb 2024 12:40:36 +0100 Subject: [PATCH] storage_service/range_to_endpoint_map: allow API to properly handle tablets This API endpoint was failing when tablets were enabled because of usage of get_vnode_effective_replication_map(). Moreover, it was providing an error message that was not user-friendly. This change extends the handler to properly service the incoming requests. Furthermore, it introduces two new test cases that verify the behavior of storage_service/range_to_endpoint_map API. It also adjusts the test case of this endpoint for vnodes to succeed when tablets are enabled by default. The new logic is as follows: - when tablets are disabled then users may query endpoints for a keyspace or for a given table in a keyspace - when tablets are enabled then users have to provide table name, because effective replication map is per-table When user does not provide table name when tablets are enabled for a given keyspace, then BAD_REQUEST is returned with a meaningful error message. Fixes: scylladb#17343 Signed-off-by: Patryk Wrobel Closes scylladb/scylladb#17372 --- api/api-doc/storage_service.json | 8 ++++++ api/storage_service.cc | 24 +++++++++++++++++- service/storage_service.cc | 11 +++------ service/storage_service.hh | 9 +++---- test/rest_api/test_storage_service.py | 35 +++++++++++++++++++++++++-- 5 files changed, 70 insertions(+), 17 deletions(-) diff --git a/api/api-doc/storage_service.json b/api/api-doc/storage_service.json index 196d3ecad28c..46197fb95ce9 100644 --- a/api/api-doc/storage_service.json +++ b/api/api-doc/storage_service.json @@ -336,6 +336,14 @@ "allowMultiple":false, "type":"boolean", "paramType":"query" + }, + { + "name":"cf", + "description":"Column family name", + "required":false, + "allowMultiple":false, + "type":"string", + "paramType":"query" } ] } diff --git a/api/storage_service.cc b/api/storage_service.cc index c7bc6c05c8cd..93e9cc4b7843 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -83,6 +84,12 @@ static void validate_table(const http_context& ctx, sstring ks_name, sstring tab } } +static void ensure_tablets_disabled(const http_context& ctx, const sstring& ks_name, const sstring& api_endpoint_path) { + if (ctx.db.local().find_keyspace(ks_name).uses_tablets()) { + throw bad_param_exception{fmt::format("{} is per-table in keyspace '{}'. Please provide table name using 'cf' parameter.", api_endpoint_path, ks_name)}; + } +} + locator::host_id validate_host_id(const sstring& param) { auto hoep = locator::host_id_or_endpoint(param, locator::host_id_or_endpoint::param_type::host_id); return hoep.id; @@ -646,8 +653,23 @@ void set_storage_service(http_context& ctx, routes& r, sharded req) -> future { auto keyspace = validate_keyspace(ctx, req->param); + auto table = req->get_query_param("cf"); + + auto erm = std::invoke([&]() -> locator::effective_replication_map_ptr { + auto& ks = ctx.db.local().find_keyspace(keyspace); + if (table.empty()) { + ensure_tablets_disabled(ctx, keyspace, "storage_service/range_to_endpoint_map"); + return ks.get_vnode_effective_replication_map(); + } else { + validate_table(ctx, keyspace, table); + + auto& cf = ctx.db.local().find_column_family(keyspace, table); + return cf.get_effective_replication_map(); + } + }); + std::vector res; - co_return stream_range_as_array(co_await ss.local().get_range_to_address_map(keyspace), + co_return stream_range_as_array(co_await ss.local().get_range_to_address_map(erm), [](const std::pair& entry){ ss::maplist_mapper m; if (entry.first.start()) { diff --git a/service/storage_service.cc b/service/storage_service.cc index 89ef524b7760..d3e0f09d46f4 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -2007,18 +2007,13 @@ future<> storage_service::bootstrap(std::unordered_set& bootstrap_tokens, } future> -storage_service::get_range_to_address_map(const sstring& keyspace) const { - return get_range_to_address_map(_db.local().find_keyspace(keyspace).get_vnode_effective_replication_map()); -} - -future> -storage_service::get_range_to_address_map(locator::vnode_effective_replication_map_ptr erm) const { +storage_service::get_range_to_address_map(locator::effective_replication_map_ptr erm) const { return get_range_to_address_map(erm, erm->get_token_metadata_ptr()->sorted_tokens()); } // Caller is responsible to hold token_metadata valid until the returned future is resolved future> -storage_service::get_range_to_address_map(locator::vnode_effective_replication_map_ptr erm, +storage_service::get_range_to_address_map(locator::effective_replication_map_ptr erm, const std::vector& sorted_tokens) const { co_return co_await construct_range_to_endpoint_map(erm, co_await get_all_ranges(sorted_tokens)); } @@ -4788,7 +4783,7 @@ storage_service::describe_ring_for_table(const sstring& keyspace_name, const sst future> storage_service::construct_range_to_endpoint_map( - locator::vnode_effective_replication_map_ptr erm, + locator::effective_replication_map_ptr erm, const dht::token_range_vector& ranges) const { std::unordered_map res; res.reserve(ranges.size()); diff --git a/service/storage_service.hh b/service/storage_service.hh index d55dcbd3b7f6..4fc7ef050cdb 100644 --- a/service/storage_service.hh +++ b/service/storage_service.hh @@ -391,11 +391,8 @@ private: future<> bootstrap(std::unordered_set& bootstrap_tokens, std::optional& cdc_gen_id, const std::optional& replacement_info); public: - - future> get_range_to_address_map(const sstring& keyspace) const; - future> get_range_to_address_map(locator::vnode_effective_replication_map_ptr erm) const; - - future> get_range_to_address_map(locator::vnode_effective_replication_map_ptr erm, + future> get_range_to_address_map(locator::effective_replication_map_ptr erm) const; + future> get_range_to_address_map(locator::effective_replication_map_ptr erm, const std::vector& sorted_tokens) const; /** @@ -431,7 +428,7 @@ public: * @return mapping of ranges to the replicas responsible for them. */ future> construct_range_to_endpoint_map( - locator::vnode_effective_replication_map_ptr erm, + locator::effective_replication_map_ptr erm, const dht::token_range_vector& ranges) const; public: virtual future<> on_join(gms::inet_address endpoint, gms::endpoint_state_ptr ep_state, gms::permit_id) override; diff --git a/test/rest_api/test_storage_service.py b/test/rest_api/test_storage_service.py index 4c26ff2bf225..5f7123b8d64c 100644 --- a/test/rest_api/test_storage_service.py +++ b/test/rest_api/test_storage_service.py @@ -423,8 +423,8 @@ def test_materialized_view_pre_scrub_snapshot(cql, this_dc, rest_api): resp = rest_api.send("GET", f"storage_service/keyspace_scrub/{keyspace}") resp.raise_for_status() -def test_range_to_endpoint_map(cql, this_dc, rest_api): - with new_test_keyspace(cql, f"WITH REPLICATION = {{ 'class' : 'NetworkTopologyStrategy', '{this_dc}' : 1 }}") as keyspace: +def test_range_to_endpoint_map_tablets_disabled_keyspace_param_only(cql, this_dc, rest_api): + with new_test_keyspace(cql, f"WITH REPLICATION = {{ 'class' : 'NetworkTopologyStrategy', '{this_dc}' : 1 }} AND TABLETS = {{ 'enabled': false }}") as keyspace: resp = rest_api.send("GET", f"storage_service/range_to_endpoint_map/{keyspace}") resp.raise_for_status() @@ -555,3 +555,34 @@ def test_storage_service_get_natural_endpoints(cql, rest_api, tablets_enabled): resp.raise_for_status() assert resp == [rest_api.host] + +@pytest.mark.xfail(reason="rest_api suite doesn't support tablets yet (#17338), run test manually") +def test_range_to_endpoint_map_tablets_enabled_keyspace_param_only(cql, rest_api): + with new_test_keyspace(cql, "WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1 } AND TABLETS = { 'enabled': true }") as keyspace: + with new_test_table(cql, keyspace, 'p int PRIMARY KEY') as table: + resp = rest_api.send("GET", f"storage_service/range_to_endpoint_map/{keyspace}") + assert resp.status_code == requests.codes.bad_request + + resp_json = resp.json() + actual_error_reason = resp_json["message"] + expected_error_reason = f"storage_service/range_to_endpoint_map is per-table in keyspace '{keyspace}'. Please provide table name using 'cf' parameter." + assert expected_error_reason == actual_error_reason + +@pytest.mark.xfail(reason="rest_api suite doesn't support tablets yet (#17338), run test manually") +@pytest.mark.parametrize("tablets_enabled", ["true", "false"]) +def test_range_to_endpoint_map_with_table_param(cql, rest_api, tablets_enabled): + with new_test_keyspace(cql, f"WITH REPLICATION = {{ 'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1 }} AND TABLETS = {{ 'enabled': {tablets_enabled} }}") as keyspace: + with new_test_table(cql, keyspace, 'p int PRIMARY KEY') as table: + cf = table.split('.')[1] + resp = rest_api.send("GET", f"storage_service/range_to_endpoint_map/{keyspace}", params={"cf": cf}) + resp.raise_for_status() + + entries_array = resp.json() + assert len(entries_array) > 0 + + expected_endpoint = [rest_api.host] + for entry in entries_array: + token_range = entry["key"] + endpoint = entry["value"] + + assert endpoint == expected_endpoint, f"Unexpected endpoint={endpoint} for token_range={token_range}. Expected endpoint was {expected_endpoint}"