From 9a24a4ee3985aa86de726c1e3259eea416d3970c Mon Sep 17 00:00:00 2001 From: Denis Yaroshevskiy Date: Thu, 15 Feb 2024 13:52:23 -0800 Subject: [PATCH] Remove enableRoutePolicyMap v2 switch (#439) Summary: Pull Request resolved: https://github.com/facebook/mcrouter/pull/439 Removing old implementation, since the new one is rolled out in memcache. Reviewed By: disylh Differential Revision: D53812885 fbshipit-source-id: 8e4b4d3189db856795418a482ca844ad58346f7f --- mcrouter/ProxyConfig-inl.h | 3 - mcrouter/routes/RootRoute.h | 4 +- mcrouter/routes/RouteHandleMap-inl.h | 22 +++---- mcrouter/routes/RouteHandleMap.h | 4 +- mcrouter/routes/RoutePolicyMap-inl.h | 67 ++------------------- mcrouter/routes/RoutePolicyMap.h | 44 +++++--------- mcrouter/routes/test/RootRouteTest.cpp | 5 -- mcrouter/routes/test/RoutePolicyMapTest.cpp | 42 ++++--------- 8 files changed, 40 insertions(+), 151 deletions(-) diff --git a/mcrouter/ProxyConfig-inl.h b/mcrouter/ProxyConfig-inl.h index 0d0a8fb27..7d29259aa 100644 --- a/mcrouter/ProxyConfig-inl.h +++ b/mcrouter/ProxyConfig-inl.h @@ -126,15 +126,12 @@ ProxyConfig::ProxyConfig( enableDeleteDistribution || enableCrossRegionDeleteRpc, "ProxyConfig: cannot disable cross-region delete rpc if distribution is disabled"); - bool enablePolicyMapV2 = readBool("enable_policy_map_v2", false); - bool enableAsyncDlBroadcast = readBool("enable_async_dl_broadcast", false); proxyRoute_ = std::make_shared>( proxy, routeSelectors, RootRouteRolloutOpts{ - .enablePolicyMapV2 = enablePolicyMapV2, .enableDeleteDistribution = enableDeleteDistribution, .enableCrossRegionDeleteRpc = enableCrossRegionDeleteRpc, .enableAsyncDlBroadcast = enableAsyncDlBroadcast}); diff --git a/mcrouter/routes/RootRoute.h b/mcrouter/routes/RootRoute.h index 73d8ece25..e4e57ae76 100644 --- a/mcrouter/routes/RootRoute.h +++ b/mcrouter/routes/RootRoute.h @@ -27,7 +27,6 @@ namespace memcache { namespace mcrouter { struct RootRouteRolloutOpts { - bool enablePolicyMapV2 = false; bool enableDeleteDistribution = false; bool enableCrossRegionDeleteRpc = true; bool enableAsyncDlBroadcast = false; @@ -49,8 +48,7 @@ class RootRoute { rhMap_( routeSelectors, opts_.default_route, - opts_.send_invalid_route_to_default, - rolloutOpts.enablePolicyMapV2), + opts_.send_invalid_route_to_default), defaultRoute_(opts_.default_route), enableDeleteDistribution_(rolloutOpts.enableDeleteDistribution), enableCrossRegionDeleteRpc_(rolloutOpts.enableCrossRegionDeleteRpc), diff --git a/mcrouter/routes/RouteHandleMap-inl.h b/mcrouter/routes/RouteHandleMap-inl.h index 0bf66cd45..cb798db49 100644 --- a/mcrouter/routes/RouteHandleMap-inl.h +++ b/mcrouter/routes/RouteHandleMap-inl.h @@ -56,14 +56,12 @@ using UniqueVectorMap = std::unordered_map< template std::shared_ptr> makePolicyMap( UniqueVectorMap& uniqueVectors, - const RouteSelectorVector& v, - bool enableRoutePolicyV2) { + const RouteSelectorVector& v) { auto it = uniqueVectors.find(v); if (it != uniqueVectors.end()) { return it->second; } - return uniqueVectors[v] = std::make_shared>( - v, enableRoutePolicyV2); + return uniqueVectors[v] = std::make_shared>(v); } } // namespace detail @@ -72,11 +70,9 @@ template RouteHandleMap::RouteHandleMap( const RouteSelectorMap& routeSelectors, const RoutingPrefix& defaultRoute, - bool sendInvalidRouteToDefault, - bool enableRoutePolicyV2) + bool sendInvalidRouteToDefault) : defaultRoute_(defaultRoute), - sendInvalidRouteToDefault_(sendInvalidRouteToDefault), - enableRoutePolicyV2_(enableRoutePolicyV2) { + sendInvalidRouteToDefault_(sendInvalidRouteToDefault) { checkLogic( routeSelectors.find(defaultRoute_) != routeSelectors.end(), "invalid default route: {}", @@ -115,16 +111,12 @@ RouteHandleMap::RouteHandleMap( // create corresponding RoutePolicyMaps detail::UniqueVectorMap uniqueVectors; - allRoutes_ = makePolicyMap(uniqueVectors, allRoutes, enableRoutePolicyV2_); + allRoutes_ = makePolicyMap(uniqueVectors, allRoutes); for (const auto& it : byRegion) { - byRegion_.emplace( - it.first, - makePolicyMap(uniqueVectors, it.second, enableRoutePolicyV2_)); + byRegion_.emplace(it.first, makePolicyMap(uniqueVectors, it.second)); } for (const auto& it : byRoute) { - byRoute_.emplace( - it.first, - makePolicyMap(uniqueVectors, it.second, enableRoutePolicyV2_)); + byRoute_.emplace(it.first, makePolicyMap(uniqueVectors, it.second)); } assert(byRoute_.find(defaultRoute_) != byRoute_.end()); diff --git a/mcrouter/routes/RouteHandleMap.h b/mcrouter/routes/RouteHandleMap.h index 9b45a519b..27026b7df 100644 --- a/mcrouter/routes/RouteHandleMap.h +++ b/mcrouter/routes/RouteHandleMap.h @@ -34,8 +34,7 @@ class RouteHandleMap { RouteHandleMap( const RouteSelectorMap& routeSelectors, const RoutingPrefix& defaultRoute, - bool sendInvalidRouteToDefault, - bool enableRoutePolicyV2); + bool sendInvalidRouteToDefault); /** * @return pointer to a precalculated vector of route handles that a request @@ -58,7 +57,6 @@ class RouteHandleMap { const std::vector> emptyV_; const RoutingPrefix& defaultRoute_; bool sendInvalidRouteToDefault_; - bool enableRoutePolicyV2_; std::shared_ptr> defaultRouteMap_; std::shared_ptr> allRoutes_; diff --git a/mcrouter/routes/RoutePolicyMap-inl.h b/mcrouter/routes/RoutePolicyMap-inl.h index 7700d58b4..f2873872d 100644 --- a/mcrouter/routes/RoutePolicyMap-inl.h +++ b/mcrouter/routes/RoutePolicyMap-inl.h @@ -49,63 +49,6 @@ std::vector> overrideItems( template RoutePolicyMap::RoutePolicyMap( - const std::vector>>& - clusters, - bool useV2) { - if (useV2) { - v2_ = RoutePolicyMapV2(clusters); - return; - } - - // wildcards of all clusters - std::vector> wildcards; - wildcards.reserve(clusters.size()); - // Trie with aggregated policies from all clusters - Trie>>> t; - - size_t clusterId = 0; - for (auto& policy : clusters) { - wildcards.push_back(policy->wildcard); - - for (auto& it : policy->policies) { - auto clusterHandlePair = std::make_pair(clusterId, it.second); - auto existing = t.find(it.first); - if (existing != t.end()) { - existing->second.push_back(std::move(clusterHandlePair)); - } else { - t.emplace(it.first, {clusterHandlePair}); - } - } - ++clusterId; - } - - ut_.emplace("", std::move(wildcards)); - // we iterate over keys in lexicographic order, so all prefixes of key will go - // before key itself - for (auto& it : t) { - auto existing = ut_.findPrefix(it.first); - // at least empty string should be there - assert(existing != ut_.end()); - ut_.emplace(it.first, detail::overrideItems(existing->second, it.second)); - } - for (auto& it : ut_) { - it.second = detail::orderedUnique(it.second); - } -} - -template -const std::vector>& -RoutePolicyMap::getTargetsForKey(folly::StringPiece key) const { - // empty means not initialized - i.e. the flag was not enabled. - if (!v2_.empty()) { - return v2_.getTargetsForKey(key); - } - auto result = ut_.findPrefix(key); - return result == ut_.end() ? emptyV_ : result->second; -} - -template -RoutePolicyMapV2::RoutePolicyMapV2( const std::vector& clusters) { typename LBRouteMap::Builder builder; @@ -129,9 +72,8 @@ RoutePolicyMapV2::RoutePolicyMapV2( template // static -auto RoutePolicyMapV2::populateWildCards( - const std::vector& clusters) - -> std::vector { +auto RoutePolicyMap::populateWildCards( + std::span clusters) -> std::vector { std::vector res; res.reserve(clusters.size()); @@ -150,10 +92,9 @@ auto RoutePolicyMapV2::populateWildCards( template // static -auto RoutePolicyMapV2::populateRoutesForKey( +auto RoutePolicyMap::populateRoutesForKey( std::string_view key, - const std::vector& clusters) - -> std::vector { + std::span clusters) -> std::vector { std::vector> res; res.reserve(clusters.size()); diff --git a/mcrouter/routes/RoutePolicyMap.h b/mcrouter/routes/RoutePolicyMap.h index 387b62cff..25a150f65 100644 --- a/mcrouter/routes/RoutePolicyMap.h +++ b/mcrouter/routes/RoutePolicyMap.h @@ -8,6 +8,7 @@ #pragma once #include +#include #include #include @@ -48,22 +49,26 @@ class PrefixSelectorRoute; * Complexity is O(min(longest key prefix in config, key length)) */ template -class RoutePolicyMapV2 { +class RoutePolicyMap { public: using SharedRoutePtr = std::shared_ptr; using SharedClusterPtr = std::shared_ptr>; - RoutePolicyMapV2() = default; - explicit RoutePolicyMapV2(const std::vector& clusters); + RoutePolicyMap() = default; + explicit RoutePolicyMap(const std::vector& clusters); - // NOTE: change to span when the migration is over + /** + * @return vector of route handles that a request with given key should be + * forwarded to. + */ const std::vector& getTargetsForKey( std::string_view key) const { // has to be there by construction auto found = ut_.findPrefix(key); // RoutePolicyMap always has wildcard which matches everything. CHECK(found != ut_.end()); - return found->value(); + return found->value(); // NOTE: returning a span would make sense here but + // it requires noticeable changes downstream. } bool empty() const { @@ -72,42 +77,21 @@ class RoutePolicyMapV2 { private: static std::vector populateWildCards( - const std::vector& clusters); + std::span clusters); static std::vector populateRoutesForKey( std::string_view key, - const std::vector& clusters); + std::span clusters); using LBRouteMap = LowerBoundPrefixMap>; - LBRouteMap ut_; -}; - -template -class RoutePolicyMap { - public: - explicit RoutePolicyMap( - const std::vector>>& - clusters, - bool useV2); - /** - * @return vector of route handles that a request with given key should be - * forwarded to. - */ - const std::vector>& getTargetsForKey( - folly::StringPiece key) const; - - private: - RoutePolicyMapV2 v2_; - const std::vector> emptyV_; - /** - * This Trie contains targets for each key prefix. It is built like this: + * This map contains targets for each key prefix. It is built like this: * 1) targets for empty string are wildcards. * 2) targets for string of length n+1 S[0..n] are targets for S[0..n-1] with * OperationSelectorRoutes for key prefix == S[0..n] overridden. */ - Trie>> ut_; + LBRouteMap ut_; }; } // namespace mcrouter diff --git a/mcrouter/routes/test/RootRouteTest.cpp b/mcrouter/routes/test/RootRouteTest.cpp index 0805138d7..549772476 100644 --- a/mcrouter/routes/test/RootRouteTest.cpp +++ b/mcrouter/routes/test/RootRouteTest.cpp @@ -265,7 +265,6 @@ TEST_F(RootRouteTest, NoRoutingPrefixDeleteWithDistributionOnRoutesToDefault) { *proxy, getRouteSelectors(), RootRouteRolloutOpts{ - .enablePolicyMapV2 = true, .enableDeleteDistribution = true, .enableCrossRegionDeleteRpc = true, }}; @@ -290,7 +289,6 @@ TEST_F(RootRouteTest, BroadcastPrefixDeleteWithDistributionOnRoutesToAll) { *proxy, getRouteSelectors(), RootRouteRolloutOpts{ - .enablePolicyMapV2 = true, .enableDeleteDistribution = true, .enableCrossRegionDeleteRpc = true, }}; @@ -323,7 +321,6 @@ TEST_F( *proxy, getRouteSelectors(), RootRouteRolloutOpts{ - .enablePolicyMapV2 = true, .enableDeleteDistribution = true, .enableCrossRegionDeleteRpc = true, }}; @@ -353,7 +350,6 @@ TEST_F( *proxy, getRouteSelectors(), RootRouteRolloutOpts{ - .enablePolicyMapV2 = true, .enableDeleteDistribution = true, .enableCrossRegionDeleteRpc = false, }}; @@ -381,7 +377,6 @@ TEST_F( *proxy, getRouteSelectors(), RootRouteRolloutOpts{ - .enablePolicyMapV2 = true, .enableDeleteDistribution = true, .enableCrossRegionDeleteRpc = false, }}; diff --git a/mcrouter/routes/test/RoutePolicyMapTest.cpp b/mcrouter/routes/test/RoutePolicyMapTest.cpp index 2f118a7ce..af20a6a68 100644 --- a/mcrouter/routes/test/RoutePolicyMapTest.cpp +++ b/mcrouter/routes/test/RoutePolicyMapTest.cpp @@ -48,38 +48,22 @@ struct MockPrefixSelectorRoute { } }; -struct MapsToTest { - explicit MapsToTest(const std::vector& clusters) - : m0(clusters, false), m1(clusters, true), m2(clusters) {} - - RoutePolicyMap m0; - RoutePolicyMap m1; - RoutePolicyMapV2 m2; -}; - -MapsToTest makeMap(const std::vector& routes) { +auto makeMap(const std::vector& routes) { std::vector clusters(routes.begin(), routes.end()); - return MapsToTest(clusters); + return RoutePolicyMap(clusters); } -std::vector routesFor(MapsToTest& maps, folly::StringPiece key) { - auto resForMap = [&](const auto& m) { - std::vector res; - auto actual = m.getTargetsForKey(key); - res.reserve(actual.size()); - for (const auto& x : actual) { - EXPECT_NE(x, nullptr); - res.push_back(x != nullptr ? *x : -1); - } - return res; - }; - - auto r0 = resForMap(maps.m0); - auto r1 = resForMap(maps.m1); - auto r2 = resForMap(maps.m2); - EXPECT_EQ(r0, r1) << key; - EXPECT_EQ(r0, r2) << key; - return r0; +std::vector routesFor( + const RoutePolicyMap& m, + folly::StringPiece key) { + std::vector res; + auto actual = m.getTargetsForKey(key); + res.reserve(actual.size()); + for (const auto& x : actual) { + EXPECT_NE(x, nullptr); + res.push_back(x != nullptr ? *x : -1); + } + return res; } TEST(RoutePolicyMapTest, NoPolicies) {