storage_service/range_to_endpoint_map: allow API to properly handle t…

…ablets

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: #17343

Signed-off-by: Patryk Wrobel <patryk.wrobel@scylladb.com>

Closes #17372

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"
                   }
                ]
             }

api/storage_service.cc

@@ -19,6 +19,7 @@
 #include <sstream>
 #include <time.h>
 #include <algorithm>
+#include <functional>
 #include <boost/range/adaptor/map.hpp>
 #include <boost/range/adaptor/filtered.hpp>
 #include <boost/algorithm/string/trim_all.hpp>
@@ -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<service::storage_
 
     ss::get_range_to_endpoint_map.set(r, [&ctx, &ss](std::unique_ptr<http::request> req) -> future<json::json_return_type> {
         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<ss::maplist_mapper> 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<dht::token_range, inet_address_vector_replica_set>& entry){
             ss::maplist_mapper m;
             if (entry.first.start()) {

service/storage_service.cc

@@ -2007,18 +2007,13 @@ future<> storage_service::bootstrap(std::unordered_set<token>& bootstrap_tokens,
 }
 
 future<std::unordered_map<dht::token_range, inet_address_vector_replica_set>>
-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<std::unordered_map<dht::token_range, inet_address_vector_replica_set>>
-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<std::unordered_map<dht::token_range, inet_address_vector_replica_set>>
-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<token>& 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<std::unordered_map<dht::token_range, inet_address_vector_replica_set>>
 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<dht::token_range, inet_address_vector_replica_set> res;
     res.reserve(ranges.size());

service/storage_service.hh

@@ -391,11 +391,8 @@ private:
     future<> bootstrap(std::unordered_set<token>& bootstrap_tokens, std::optional<cdc::generation_id>& cdc_gen_id, const std::optional<replacement_info>& replacement_info);
 
 public:
-
-    future<std::unordered_map<dht::token_range, inet_address_vector_replica_set>> get_range_to_address_map(const sstring& keyspace) const;
-    future<std::unordered_map<dht::token_range, inet_address_vector_replica_set>> get_range_to_address_map(locator::vnode_effective_replication_map_ptr erm) const;
-
-    future<std::unordered_map<dht::token_range, inet_address_vector_replica_set>> get_range_to_address_map(locator::vnode_effective_replication_map_ptr erm,
+    future<std::unordered_map<dht::token_range, inet_address_vector_replica_set>> get_range_to_address_map(locator::effective_replication_map_ptr erm) const;
+    future<std::unordered_map<dht::token_range, inet_address_vector_replica_set>> get_range_to_address_map(locator::effective_replication_map_ptr erm,
             const std::vector<token>& sorted_tokens) const;
 
     /**
@@ -431,7 +428,7 @@ public:
      * @return mapping of ranges to the replicas responsible for them.
     */
     future<std::unordered_map<dht::token_range, inet_address_vector_replica_set>> 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;

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}"