Remove enableRoutePolicyMap v2 switch (#439)

Summary: Pull Request resolved: #439 Removing old implementation, since the new one is rolled out in memcache. Reviewed By: disylh Differential Revision: D53812885 fbshipit-source-id: 8e4b4d3189db856795418a482ca844ad58346f7f
facebook · Feb 15, 2024 · 9a24a4e · 9a24a4e
1 parent fa880c5
commit 9a24a4e
Show file tree

Hide file tree

Showing 8 changed files with 40 additions and 151 deletions.
diff --git a/mcrouter/ProxyConfig-inl.h b/mcrouter/ProxyConfig-inl.h
@@ -126,15 +126,12 @@ ProxyConfig<RouterInfo>::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<ProxyRoute<RouterInfo>>(
       proxy,
       routeSelectors,
       RootRouteRolloutOpts{
-          .enablePolicyMapV2 = enablePolicyMapV2,
           .enableDeleteDistribution = enableDeleteDistribution,
           .enableCrossRegionDeleteRpc = enableCrossRegionDeleteRpc,
           .enableAsyncDlBroadcast = enableAsyncDlBroadcast});

diff --git 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
@@ -56,14 +56,12 @@ using UniqueVectorMap = std::unordered_map<
 template <class RouteHandleIf>
 std::shared_ptr<RoutePolicyMap<RouteHandleIf>> makePolicyMap(
     UniqueVectorMap<RouteHandleIf>& uniqueVectors,
-    const RouteSelectorVector<RouteHandleIf>& v,
-    bool enableRoutePolicyV2) {
+    const RouteSelectorVector<RouteHandleIf>& v) {
   auto it = uniqueVectors.find(v);
   if (it != uniqueVectors.end()) {
     return it->second;
   }
-  return uniqueVectors[v] = std::make_shared<RoutePolicyMap<RouteHandleIf>>(
-             v, enableRoutePolicyV2);
+  return uniqueVectors[v] = std::make_shared<RoutePolicyMap<RouteHandleIf>>(v);
 }
 
 } // namespace detail
@@ -72,11 +70,9 @@ template <class RouteHandleIf>
 RouteHandleMap<RouteHandleIf>::RouteHandleMap(
     const RouteSelectorMap<RouteHandleIf>& 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<RouteHandleIf>::RouteHandleMap(
 
   // create corresponding RoutePolicyMaps
   detail::UniqueVectorMap<RouteHandleIf> 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
@@ -34,8 +34,7 @@ class RouteHandleMap {
   RouteHandleMap(
       const RouteSelectorMap<RouteHandleIf>& 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<std::shared_ptr<RouteHandleIf>> emptyV_;
   const RoutingPrefix& defaultRoute_;
   bool sendInvalidRouteToDefault_;
-  bool enableRoutePolicyV2_;
   std::shared_ptr<RoutePolicyMap<RouteHandleIf>> defaultRouteMap_;
 
   std::shared_ptr<RoutePolicyMap<RouteHandleIf>> allRoutes_;

diff --git a/mcrouter/routes/RoutePolicyMap-inl.h b/mcrouter/routes/RoutePolicyMap-inl.h
@@ -49,63 +49,6 @@ std::vector<std::shared_ptr<RouteHandleIf>> overrideItems(
 
 template <class RouteHandleIf>
 RoutePolicyMap<RouteHandleIf>::RoutePolicyMap(
-    const std::vector<std::shared_ptr<PrefixSelectorRoute<RouteHandleIf>>>&
-        clusters,
-    bool useV2) {
-  if (useV2) {
-    v2_ = RoutePolicyMapV2<RouteHandleIf>(clusters);
-    return;
-  }
-
-  // wildcards of all clusters
-  std::vector<std::shared_ptr<RouteHandleIf>> wildcards;
-  wildcards.reserve(clusters.size());
-  // Trie with aggregated policies from all clusters
-  Trie<std::vector<std::pair<size_t, std::shared_ptr<RouteHandleIf>>>> 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 <class RouteHandleIf>
-const std::vector<std::shared_ptr<RouteHandleIf>>&
-RoutePolicyMap<RouteHandleIf>::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 <class RouteHandleIf>
-RoutePolicyMapV2<RouteHandleIf>::RoutePolicyMapV2(
     const std::vector<SharedClusterPtr>& clusters) {
   typename LBRouteMap::Builder builder;
 
@@ -129,9 +72,8 @@ RoutePolicyMapV2<RouteHandleIf>::RoutePolicyMapV2(
 
 template <class RouteHandleIf>
 // static
-auto RoutePolicyMapV2<RouteHandleIf>::populateWildCards(
-    const std::vector<SharedClusterPtr>& clusters)
-    -> std::vector<SharedRoutePtr> {
+auto RoutePolicyMap<RouteHandleIf>::populateWildCards(
+    std::span<const SharedClusterPtr> clusters) -> std::vector<SharedRoutePtr> {
   std::vector<SharedRoutePtr> res;
   res.reserve(clusters.size());
 
@@ -150,10 +92,9 @@ auto RoutePolicyMapV2<RouteHandleIf>::populateWildCards(
 
 template <class RouteHandleIf>
 // static
-auto RoutePolicyMapV2<RouteHandleIf>::populateRoutesForKey(
+auto RoutePolicyMap<RouteHandleIf>::populateRoutesForKey(
     std::string_view key,
-    const std::vector<SharedClusterPtr>& clusters)
-    -> std::vector<SharedRoutePtr> {
+    std::span<const SharedClusterPtr> clusters) -> std::vector<SharedRoutePtr> {
   std::vector<std::shared_ptr<RouteHandleIf>> res;
   res.reserve(clusters.size());
 

diff --git a/mcrouter/routes/RoutePolicyMap.h b/mcrouter/routes/RoutePolicyMap.h
@@ -8,6 +8,7 @@
 #pragma once
 
 #include <memory>
+#include <span>
 #include <string_view>
 #include <vector>
 
@@ -48,22 +49,26 @@ class PrefixSelectorRoute;
  *    Complexity is O(min(longest key prefix in config, key length))
  */
 template <class RouteHandleIf>
-class RoutePolicyMapV2 {
+class RoutePolicyMap {
  public:
   using SharedRoutePtr = std::shared_ptr<RouteHandleIf>;
   using SharedClusterPtr = std::shared_ptr<PrefixSelectorRoute<RouteHandleIf>>;
 
-  RoutePolicyMapV2() = default;
-  explicit RoutePolicyMapV2(const std::vector<SharedClusterPtr>& clusters);
+  RoutePolicyMap() = default;
+  explicit RoutePolicyMap(const std::vector<SharedClusterPtr>& 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<SharedRoutePtr>& 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<SharedRoutePtr> populateWildCards(
-      const std::vector<SharedClusterPtr>& clusters);
+      std::span<const SharedClusterPtr> clusters);
 
   static std::vector<SharedRoutePtr> populateRoutesForKey(
       std::string_view key,
-      const std::vector<SharedClusterPtr>& clusters);
+      std::span<const SharedClusterPtr> clusters);
 
   using LBRouteMap = LowerBoundPrefixMap<std::vector<SharedRoutePtr>>;
 
-  LBRouteMap ut_;
-};
-
-template <class RouteHandleIf>
-class RoutePolicyMap {
- public:
-  explicit RoutePolicyMap(
-      const std::vector<std::shared_ptr<PrefixSelectorRoute<RouteHandleIf>>>&
-          clusters,
-      bool useV2);
-
   /**
-   * @return vector of route handles that a request with given key should be
-   *         forwarded to.
-   */
-  const std::vector<std::shared_ptr<RouteHandleIf>>& getTargetsForKey(
-      folly::StringPiece key) const;
-
- private:
-  RoutePolicyMapV2<RouteHandleIf> v2_;
-  const std::vector<std::shared_ptr<RouteHandleIf>> 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<std::vector<std::shared_ptr<RouteHandleIf>>> ut_;
+  LBRouteMap ut_;
 };
 
 } // namespace mcrouter

diff --git 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
@@ -48,38 +48,22 @@ struct MockPrefixSelectorRoute {
   }
 };
 
-struct MapsToTest {
-  explicit MapsToTest(const std::vector<SharedSelector>& clusters)
-      : m0(clusters, false), m1(clusters, true), m2(clusters) {}
-
-  RoutePolicyMap<RouteHandle> m0;
-  RoutePolicyMap<RouteHandle> m1;
-  RoutePolicyMapV2<RouteHandle> m2;
-};
-
-MapsToTest makeMap(const std::vector<MockPrefixSelectorRoute>& routes) {
+auto makeMap(const std::vector<MockPrefixSelectorRoute>& routes) {
   std::vector<SharedSelector> clusters(routes.begin(), routes.end());
-  return MapsToTest(clusters);
+  return RoutePolicyMap<RouteHandle>(clusters);
 }
 
-std::vector<int> routesFor(MapsToTest& maps, folly::StringPiece key) {
-  auto resForMap = [&](const auto& m) {
-    std::vector<int> 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<int> routesFor(
+    const RoutePolicyMap<RouteHandle>& m,
+    folly::StringPiece key) {
+  std::vector<int> 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) {