From f91c5ce5b1e4ca362add418a8b32de7a23bfcd9e Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Mon, 24 Sep 2018 11:25:56 -0700
Subject: [PATCH 01/13] retry extensions: implement other priority extension

Implements a RetryPriority which will keep track of attempted
priorities and attempt to route retry requests to other priorities. The
update frequency is configurable, allowing multiple requests to hit each
priority if desired.

As a fallback, when no healthy priorities remain, the list of attempted
priorities will be reset and a host will selected again using the
original priority load.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 api/envoy/config/retry/other_priority/BUILD   |  11 +
 .../other_priority_config.proto               |  11 +
 include/envoy/upstream/retry.h                |   4 +
 source/common/router/config_impl.cc           |  16 +-
 source/common/upstream/load_balancer_impl.cc  |  42 ++--
 source/common/upstream/load_balancer_impl.h   |   5 +-
 source/extensions/extensions_build_config.bzl |   3 +
 .../retry/host/other_hosts/config.h           |   4 +
 source/extensions/retry/priority/BUILD        |  17 ++
 .../retry/priority/other_priority/BUILD       |  33 +++
 .../retry/priority/other_priority/config.cc   |  26 +++
 .../retry/priority/other_priority/config.h    |  32 +++
 .../priority/other_priority/other_priority.cc |  98 +++++++++
 .../priority/other_priority/other_priority.h  |  52 +++++
 .../retry/priority/well_known_names.h         |  24 +++
 .../retry/priority/other_priority/BUILD       |  23 ++
 .../priority/other_priority/config_test.cc    | 196 ++++++++++++++++++
 test/integration/test_host_predicate_config.h |   3 +
 test/mocks/upstream/mocks.h                   |   3 +
 19 files changed, 578 insertions(+), 25 deletions(-)
 create mode 100644 api/envoy/config/retry/other_priority/BUILD
 create mode 100644 api/envoy/config/retry/other_priority/other_priority_config.proto
 create mode 100644 source/extensions/retry/priority/BUILD
 create mode 100644 source/extensions/retry/priority/other_priority/BUILD
 create mode 100644 source/extensions/retry/priority/other_priority/config.cc
 create mode 100644 source/extensions/retry/priority/other_priority/config.h
 create mode 100644 source/extensions/retry/priority/other_priority/other_priority.cc
 create mode 100644 source/extensions/retry/priority/other_priority/other_priority.h
 create mode 100644 source/extensions/retry/priority/well_known_names.h
 create mode 100644 test/extensions/retry/priority/other_priority/BUILD
 create mode 100644 test/extensions/retry/priority/other_priority/config_test.cc

diff --git a/api/envoy/config/retry/other_priority/BUILD b/api/envoy/config/retry/other_priority/BUILD
new file mode 100644
index 0000000000000..cb0cfcd025394
--- /dev/null
+++ b/api/envoy/config/retry/other_priority/BUILD
@@ -0,0 +1,11 @@
+licenses(["notice"])  # Apache 2
+
+load("//bazel:api_build_system.bzl", "api_proto_library_internal")
+
+api_proto_library_internal(
+    name = "other_priority",
+    srcs = ["other_priority_config.proto"],
+    deps = [
+        "//envoy/api/v2/core:base",
+    ],
+)
diff --git a/api/envoy/config/retry/other_priority/other_priority_config.proto b/api/envoy/config/retry/other_priority/other_priority_config.proto
new file mode 100644
index 0000000000000..d7e41f9b52bf1
--- /dev/null
+++ b/api/envoy/config/retry/other_priority/other_priority_config.proto
@@ -0,0 +1,11 @@
+syntax = "proto3";
+
+package envoy.config.retry.other_priority;
+
+// A retry host selector that attempts to spread retries between priorities, even if certain
+// priorities would not normally be attempted due to higher priorities being available.
+message OtherPriorityConfig {
+  // How often the priority load should be updated based on previously attempted priorities. Useful
+  // to allow each priorities to receive more than one request before being excluded.
+  int32 update_frequency = 1;
+}
diff --git a/include/envoy/upstream/retry.h b/include/envoy/upstream/retry.h
index 34b9ef391e1dd..81dba4f301230 100644
--- a/include/envoy/upstream/retry.h
+++ b/include/envoy/upstream/retry.h
@@ -109,6 +109,8 @@ class RetryPriorityFactory {
                                    const Protobuf::Message& config) PURE;
 
   virtual std::string name() const PURE;
+
+  virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE;
 };
 
 /**
@@ -125,6 +127,8 @@ class RetryHostPredicateFactory {
    * @return name name of this factory.
    */
   virtual std::string name() PURE;
+
+  virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE;
 };
 
 } // namespace Upstream
diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
index 4e3339717d9db..fa3c5fd1b47ee 100644
--- a/source/common/router/config_impl.cc
+++ b/source/common/router/config_impl.cc
@@ -50,15 +50,21 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::api::v2::route::RouteAction& confi
   retry_on_ |= RetryStateImpl::parseRetryGrpcOn(config.retry_policy().retry_on());
 
   for (const auto& host_predicate : config.retry_policy().retry_host_predicate()) {
-    Registry::FactoryRegistry<Upstream::RetryHostPredicateFactory>::getFactory(
-        host_predicate.name())
-        ->createHostPredicate(*this, host_predicate.config());
+    auto& factory =
+        ::Envoy::Config::Utility::getAndCheckFactory<Upstream::RetryHostPredicateFactory>(
+            host_predicate.name());
+
+    auto config = ::Envoy::Config::Utility::translateToFactoryConfig(host_predicate, factory);
+    factory.createHostPredicate(*this, *config);
   }
 
   const auto retry_priority = config.retry_policy().retry_priority();
   if (!retry_priority.name().empty()) {
-    Registry::FactoryRegistry<Upstream::RetryPriorityFactory>::getFactory(retry_priority.name())
-        ->createRetryPriority(*this, retry_priority.config());
+    auto& factory = ::Envoy::Config::Utility::getAndCheckFactory<Upstream::RetryPriorityFactory>(
+        retry_priority.name());
+
+    auto config = ::Envoy::Config::Utility::translateToFactoryConfig(retry_priority, factory);
+    factory.createRetryPriority(*this, *config);
   }
 
   auto host_selection_attempts = config.retry_policy().host_selection_retry_max_attempts();
diff --git a/source/common/upstream/load_balancer_impl.cc b/source/common/upstream/load_balancer_impl.cc
index 94cea9fcf9a13..1c19a54ca229c 100644
--- a/source/common/upstream/load_balancer_impl.cc
+++ b/source/common/upstream/load_balancer_impl.cc
@@ -44,25 +44,29 @@ LoadBalancerBase::LoadBalancerBase(const PrioritySet& priority_set, ClusterStats
           common_config, healthy_panic_threshold, 100, 50)),
       priority_set_(priority_set) {
   for (auto& host_set : priority_set_.hostSetsPerPriority()) {
-    recalculatePerPriorityState(host_set->priority());
+    recalculatePerPriorityState(host_set->priority(), priority_set_, per_priority_load_,
+                                per_priority_health_);
   }
-  priority_set_.addMemberUpdateCb(
-      [this](uint32_t priority, const HostVector&, const HostVector&) -> void {
-        recalculatePerPriorityState(priority);
-      });
+  priority_set_.addMemberUpdateCb([this](uint32_t priority, const HostVector&,
+                                         const HostVector&) -> void {
+    recalculatePerPriorityState(priority, priority_set_, per_priority_load_, per_priority_health_);
+  });
 }
 
-void LoadBalancerBase::recalculatePerPriorityState(uint32_t priority) {
-  per_priority_load_.resize(priority_set_.hostSetsPerPriority().size());
-  per_priority_health_.resize(priority_set_.hostSetsPerPriority().size());
+void LoadBalancerBase::recalculatePerPriorityState(uint32_t priority,
+                                                   const PrioritySet& priority_set,
+                                                   PriorityLoad& per_priority_load,
+                                                   std::vector<uint32_t>& per_priority_health) {
+  per_priority_load.resize(priority_set.hostSetsPerPriority().size());
+  per_priority_health.resize(priority_set.hostSetsPerPriority().size());
 
   // Determine the health of the newly modified priority level.
   // Health ranges from 0-100, and is the ratio of healthy hosts to total hosts, modified by the
   // overprovisioning factor.
-  HostSet& host_set = *priority_set_.hostSetsPerPriority()[priority];
-  per_priority_health_[priority] = 0;
+  HostSet& host_set = *priority_set.hostSetsPerPriority()[priority];
+  per_priority_health[priority] = 0;
   if (host_set.hosts().size() > 0) {
-    per_priority_health_[priority] =
+    per_priority_health[priority] =
         std::min<uint32_t>(100, (host_set.overprovisioning_factor() *
                                  host_set.healthyHosts().size() / host_set.hosts().size()));
   }
@@ -74,25 +78,25 @@ void LoadBalancerBase::recalculatePerPriorityState(uint32_t priority) {
   // 3 host sets with 20% / 20% / 10% health they will get 40% / 40% / 20% load to ensure total load
   // adds up to 100.
   const uint32_t total_health = std::min<uint32_t>(
-      std::accumulate(per_priority_health_.begin(), per_priority_health_.end(), 0), 100);
+      std::accumulate(per_priority_health.begin(), per_priority_health.end(), 0), 100);
   if (total_health == 0) {
     // Everything is terrible. Send all load to P=0.
     // In this one case sumEntries(per_priority_load_) != 100 since we sinkhole all traffic in P=0.
-    per_priority_load_[0] = 100;
+    per_priority_load[0] = 100;
     return;
   }
   size_t total_load = 100;
-  for (size_t i = 0; i < per_priority_health_.size(); ++i) {
+  for (size_t i = 0; i < per_priority_health.size(); ++i) {
     // Now assign as much load as possible to the high priority levels and cease assigning load when
     // total_load runs out.
-    per_priority_load_[i] =
-        std::min<uint32_t>(total_load, per_priority_health_[i] * 100 / total_health);
-    total_load -= per_priority_load_[i];
+    per_priority_load[i] =
+        std::min<uint32_t>(total_load, per_priority_health[i] * 100 / total_health);
+    total_load -= per_priority_load[i];
   }
   if (total_load != 0) {
     // Account for rounding errors.
-    ASSERT(total_load < per_priority_load_.size());
-    per_priority_load_[0] += total_load;
+    ASSERT(total_load < per_priority_load.size());
+    per_priority_load[0] += total_load;
   }
 }
 
diff --git a/source/common/upstream/load_balancer_impl.h b/source/common/upstream/load_balancer_impl.h
index 7d165e9d31aac..5b0baff811af2 100644
--- a/source/common/upstream/load_balancer_impl.h
+++ b/source/common/upstream/load_balancer_impl.h
@@ -70,10 +70,13 @@ class LoadBalancerBase : public LoadBalancer {
   // The priority-ordered set of hosts to use for load balancing.
   const PrioritySet& priority_set_;
 
+public:
   // Called when a host set at the given priority level is updated. This updates
   // per_priority_health_ for that priority level, and may update per_priority_load_ for all
   // priority levels.
-  void recalculatePerPriorityState(uint32_t priority);
+  void static recalculatePerPriorityState(uint32_t priority, const PrioritySet& priority_set,
+                                          PriorityLoad& priority_load,
+                                          std::vector<uint32_t>& per_priority_health_);
 
   // The percentage load (0-100) for each priority level
   std::vector<uint32_t> per_priority_load_;
diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl
index eaf1045aaba54..000743b3587dc 100644
--- a/source/extensions/extensions_build_config.bzl
+++ b/source/extensions/extensions_build_config.bzl
@@ -111,6 +111,9 @@ EXTENSIONS = {
 
     # Retry host predicates
     "envoy.retry_host_predicates.other_hosts":          "//source/extensions/retry/host/other_hosts:config",
+
+    # Retry priorities
+    "envoy.retry_priorities.previous_priorities":       "//source/extensions/retry/priority/other_priority:config",
 }
 
 WINDOWS_EXTENSIONS = {
diff --git a/source/extensions/retry/host/other_hosts/config.h b/source/extensions/retry/host/other_hosts/config.h
index 2d0ee8a1c1447..e0206497f2f69 100644
--- a/source/extensions/retry/host/other_hosts/config.h
+++ b/source/extensions/retry/host/other_hosts/config.h
@@ -18,6 +18,10 @@ class OtherHostsRetryPredicateFactory : public Upstream::RetryHostPredicateFacto
   }
 
   std::string name() override { return RetryHostPredicateValues::get().PreviousHostsPredicate; }
+
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()};
+  }
 };
 
 } // namespace Host
diff --git a/source/extensions/retry/priority/BUILD b/source/extensions/retry/priority/BUILD
new file mode 100644
index 0000000000000..6156949edef64
--- /dev/null
+++ b/source/extensions/retry/priority/BUILD
@@ -0,0 +1,17 @@
+licenses(["notice"])  # Apache 2
+
+load(
+    "//bazel:envoy_build_system.bzl",
+    "envoy_cc_library",
+    "envoy_package",
+)
+
+envoy_package()
+
+envoy_cc_library(
+    name = "well_known_names",
+    hdrs = ["well_known_names.h"],
+    deps = [
+        "//source/common/singleton:const_singleton",
+    ],
+)
diff --git a/source/extensions/retry/priority/other_priority/BUILD b/source/extensions/retry/priority/other_priority/BUILD
new file mode 100644
index 0000000000000..6014100d4b4a0
--- /dev/null
+++ b/source/extensions/retry/priority/other_priority/BUILD
@@ -0,0 +1,33 @@
+licenses(["notice"])  # Apache 2
+
+load(
+    "//bazel:envoy_build_system.bzl",
+    "envoy_cc_library",
+    "envoy_package",
+)
+
+envoy_package()
+
+envoy_cc_library(
+    name = "other_priority_lib",
+    srcs = ["other_priority.cc"],
+    hdrs = ["other_priority.h"],
+    deps = [
+        "//include/envoy/upstream:retry_interface",
+        "//source/common/upstream:load_balancer_lib",
+    ],
+)
+
+envoy_cc_library(
+    name = "config",
+    srcs = ["config.cc"],
+    hdrs = ["config.h"],
+    deps = [
+        ":other_priority_lib",
+        "//include/envoy/registry",
+        "//include/envoy/upstream:retry_interface",
+        "//source/common/protobuf",
+        "//source/extensions/retry/priority:well_known_names",
+        "@envoy_api//envoy/config/retry/other_priority:other_priority_cc",
+    ],
+)
diff --git a/source/extensions/retry/priority/other_priority/config.cc b/source/extensions/retry/priority/other_priority/config.cc
new file mode 100644
index 0000000000000..abeb70facd25b
--- /dev/null
+++ b/source/extensions/retry/priority/other_priority/config.cc
@@ -0,0 +1,26 @@
+#include "extensions/retry/priority/other_priority/config.h"
+
+#include "envoy/config/retry/other_priority/other_priority_config.pb.validate.h"
+#include "envoy/registry/registry.h"
+#include "envoy/upstream/retry.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace Retry {
+namespace Priority {
+
+void OtherPriorityRetryPriorityFactory::createRetryPriority(
+    Upstream::RetryPriorityFactoryCallbacks& callbacks, const Protobuf::Message& config) {
+  callbacks.addRetryPriority(std::make_shared<OtherPriorityRetryPriority>(
+      MessageUtil::downcastAndValidate<
+          const envoy::config::retry::other_priority::OtherPriorityConfig&>(config)
+          .update_frequency()));
+}
+
+static Registry::RegisterFactory<OtherPriorityRetryPriorityFactory, Upstream::RetryPriorityFactory>
+    register_;
+
+} // namespace Priority
+} // namespace Retry
+} // namespace Extensions
+} // namespace Envoy
diff --git a/source/extensions/retry/priority/other_priority/config.h b/source/extensions/retry/priority/other_priority/config.h
new file mode 100644
index 0000000000000..fe2ea0d4b48cf
--- /dev/null
+++ b/source/extensions/retry/priority/other_priority/config.h
@@ -0,0 +1,32 @@
+#pragma once
+
+#include "envoy/upstream/retry.h"
+
+#include "common/protobuf/protobuf.h"
+
+#include "extensions/retry/priority/other_priority/other_priority.h"
+#include "extensions/retry/priority/well_known_names.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace Retry {
+namespace Priority {
+
+class OtherPriorityRetryPriorityFactory : public Upstream::RetryPriorityFactory {
+public:
+  void createRetryPriority(Upstream::RetryPriorityFactoryCallbacks& callbacks,
+                           const Protobuf::Message& config) override;
+
+  std::string name() const override {
+    return RetryPriorityValues::get().PreviousPrioritiesRetryPriority;
+  }
+
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr(new ::Envoy::ProtobufWkt::Empty());
+  }
+};
+
+} // namespace Priority
+} // namespace Retry
+} // namespace Extensions
+} // namespace Envoy
diff --git a/source/extensions/retry/priority/other_priority/other_priority.cc b/source/extensions/retry/priority/other_priority/other_priority.cc
new file mode 100644
index 0000000000000..9fa55beb114ec
--- /dev/null
+++ b/source/extensions/retry/priority/other_priority/other_priority.cc
@@ -0,0 +1,98 @@
+#include "extensions/retry/priority/other_priority/other_priority.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace Retry {
+namespace Priority {
+const Upstream::PriorityLoad& OtherPriorityRetryPriority::determinePriorityLoad(
+    const Upstream::PrioritySet& priority_set,
+    const Upstream::PriorityLoad& original_priority_load) {
+  if (!priority_set_) {
+    priority_set_ = &priority_set;
+
+    excluded_priorities_.resize(original_priority_load.size());
+    // Initialize our local priority_load_ and priority_health_,
+    // keeping them in sync with the member update cb.
+    for (auto& host_set : priority_set_->hostSetsPerPriority()) {
+      recalculatePerPriorityState(host_set->priority());
+    }
+    callback_ = priority_set.addMemberUpdateCb([&,this](int priority, auto, auto) {
+      recalculatePerPriorityState(priority);
+      adjustForAttemptedPriorities();
+    });
+  }
+
+  // If we've not seen enough retries to modify the priority load, just
+  // return the original.
+  // If this retry should trigger an update, recalculate the priority load by excluding attempted
+  // priorities.
+  if (attempted_priorites_.size() < update_frequency_) {
+    return original_priority_load;
+  } else if (attempted_priorites_.size() % update_frequency_ == 0) {
+    for (auto priority : attempted_priorites_) {
+      excluded_priorities_[priority] = true;
+    }
+
+    adjustForAttemptedPriorities();
+  }
+
+  return per_priority_load_;
+}
+
+void OtherPriorityRetryPriority::adjustForAttemptedPriorities() {
+  // If all priorities are unhealthy to begin with, there's nothing to do.
+  if (!std::accumulate(per_priority_load_.begin(), per_priority_load_.end(), 0)) {
+    return;
+  }
+
+  // First we create an adjusted health view of the priorities, where attempted priorities are
+  // given a zero weight.
+  uint32_t total_health = 0;
+  std::vector<uint32_t> adjusted_per_priority_health;
+  adjusted_per_priority_health.resize(per_priority_health_.size());
+  for (size_t i = 0; i < per_priority_health_.size(); ++i) {
+    if (!excluded_priorities_[i]) {
+      adjusted_per_priority_health[i] = per_priority_health_[i];
+      total_health += per_priority_health_[i];
+    }
+  }
+
+  total_health = std::min(total_health, 100u);
+
+  // If there are no healthy priorities left, we reset the attempted priorities and recurse.
+  // This allows us to fall back to the unmodified priority load when we run out of priorites
+  // instead of failing to route requests.
+  if (total_health == 0) {
+    for (size_t i = 0; i < excluded_priorities_.size(); ++i) {
+      excluded_priorities_[i] = false;
+    }
+    attempted_priorites_.clear();
+
+    adjustForAttemptedPriorities();
+    return;
+  }
+
+  // We then adjust the load by rebalancing priorities with the adjusted health values.
+  Upstream::PriorityLoad per_priority_load;
+  per_priority_load.resize(per_priority_load_.size());
+
+  size_t total_load = 100;
+  // The outer loop is used to eliminate rounding errors: any remaining load will be assigned to the
+  // first healthy priority.
+  while (total_load != 0) {
+    for (size_t i = 0; i < adjusted_per_priority_health.size(); ++i) {
+      // Now assign as much load as possible to the high priority levels and cease assigning load
+      // when total_load runs out.
+      auto delta =
+          std::min<uint32_t>(total_load, adjusted_per_priority_health[i] * 100 / total_health);
+      per_priority_load[i] += delta;
+      total_load -= delta;
+    }
+  }
+
+  per_priority_load_ = per_priority_load;
+}
+} // namespace Priority
+} // namespace Retry
+} // namespace Extensions
+} // namespace Envoy
diff --git a/source/extensions/retry/priority/other_priority/other_priority.h b/source/extensions/retry/priority/other_priority/other_priority.h
new file mode 100644
index 0000000000000..a55d583bfcdae
--- /dev/null
+++ b/source/extensions/retry/priority/other_priority/other_priority.h
@@ -0,0 +1,52 @@
+#pragma once
+
+#include "envoy/upstream/retry.h"
+
+#include "common/upstream/load_balancer_impl.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace Retry {
+namespace Priority {
+
+class OtherPriorityRetryPriority : public Upstream::RetryPriority {
+public:
+  OtherPriorityRetryPriority(uint32_t update_frequency) : update_frequency_(update_frequency) {}
+  ~OtherPriorityRetryPriority() {
+    if (callback_) {
+      callback_->remove();
+    }
+  }
+
+  const Upstream::PriorityLoad&
+  determinePriorityLoad(const Upstream::PrioritySet& priority_set,
+                        const Upstream::PriorityLoad& original_priority) override;
+
+  void onHostAttempted(Upstream::HostDescriptionConstSharedPtr attempted_host) override {
+    attempted_priorites_.emplace_back(attempted_host->priority());
+  }
+
+private:
+  void recalculatePerPriorityState(uint32_t priority) {
+    // Recalcuate health and priority the same way the load balancer does it.
+    Upstream::LoadBalancerBase::recalculatePerPriorityState(
+        priority, *priority_set_, per_priority_load_, per_priority_health_);
+  }
+
+  // Distributes priority load between priorities that should be consider after
+  // excluding attempted priorities.
+  void adjustForAttemptedPriorities();
+
+  uint32_t update_frequency_;
+  std::vector<uint32_t> attempted_priorites_;
+  std::vector<bool> excluded_priorities_;
+  Upstream::PriorityLoad per_priority_load_;
+  Upstream::PrioritySet const* priority_set_{};
+  Common::CallbackHandle* callback_{};
+  std::vector<uint32_t> per_priority_health_;
+};
+
+} // namespace Priority
+} // namespace Retry
+} // namespace Extensions
+} // namespace Envoy
diff --git a/source/extensions/retry/priority/well_known_names.h b/source/extensions/retry/priority/well_known_names.h
new file mode 100644
index 0000000000000..ca99636f2962c
--- /dev/null
+++ b/source/extensions/retry/priority/well_known_names.h
@@ -0,0 +1,24 @@
+#pragma once
+
+#include "common/singleton/const_singleton.h"
+
+namespace Envoy {
+namespace Extensions {
+namespace Retry {
+namespace Priority {
+
+/**
+ * Well-known retry priority load names.
+ */
+class RetryPriorityNameValues {
+public:
+  // Previous host predicate. Rejects hosts that have already been tried.
+  const std::string PreviousPrioritiesRetryPriority = "envoy.retry_priorities.previous_priorities";
+};
+
+typedef ConstSingleton<RetryPriorityNameValues> RetryPriorityValues;
+
+} // namespace Priority
+} // namespace Retry
+} // namespace Extensions
+} // namespace Envoy
diff --git a/test/extensions/retry/priority/other_priority/BUILD b/test/extensions/retry/priority/other_priority/BUILD
new file mode 100644
index 0000000000000..59840223e9228
--- /dev/null
+++ b/test/extensions/retry/priority/other_priority/BUILD
@@ -0,0 +1,23 @@
+licenses(["notice"])  # Apache 2
+
+load(
+    "//bazel:envoy_build_system.bzl",
+    "envoy_package",
+)
+load(
+    "//test/extensions:extensions_build_system.bzl",
+    "envoy_extension_cc_test",
+)
+
+envoy_package()
+
+envoy_extension_cc_test(
+    name = "config_test",
+    srcs = ["config_test.cc"],
+    extension_name = "envoy.retry_priorities.previous_priorities",
+    deps = [
+        "//source/extensions/retry/priority:well_known_names",
+        "//source/extensions/retry/priority/other_priority:config",
+        "//test/mocks/upstream:upstream_mocks",
+    ],
+)
diff --git a/test/extensions/retry/priority/other_priority/config_test.cc b/test/extensions/retry/priority/other_priority/config_test.cc
new file mode 100644
index 0000000000000..475c28be0f9c3
--- /dev/null
+++ b/test/extensions/retry/priority/other_priority/config_test.cc
@@ -0,0 +1,196 @@
+#include "envoy/config/retry/other_priority/other_priority_config.pb.validate.h"
+#include "envoy/registry/registry.h"
+#include "envoy/upstream/retry.h"
+
+#include "extensions/retry/priority/other_priority/config.h"
+#include "extensions/retry/priority/well_known_names.h"
+
+#include "test/mocks/upstream/mocks.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace testing;
+
+namespace Envoy {
+namespace Extensions {
+namespace Retry {
+namespace Priority {
+
+class RetryPriorityTest : public ::testing::Test, Upstream::RetryPriorityFactoryCallbacks {
+public:
+  void initialize() {
+    auto factory = Registry::FactoryRegistry<Upstream::RetryPriorityFactory>::getFactory(
+        RetryPriorityValues::get().PreviousPrioritiesRetryPriority);
+
+    envoy::config::retry::other_priority::OtherPriorityConfig config;
+    config.set_update_frequency(update_frequency_);
+    factory->createRetryPriority(*this, config);
+  }
+
+  // Upstream::RetryPriorityFactoryCallbacks
+  void addRetryPriority(Upstream::RetryPrioritySharedPtr retry_priority) override {
+    retry_priority_ = retry_priority;
+  }
+
+  void addHosts(size_t priority, int count, int healthy_count) {
+    auto host_set = priority_set_.getMockHostSet(priority);
+
+    host_set->hosts_.resize(count);
+    host_set->healthy_hosts_.resize(healthy_count);
+    host_set->runCallbacks({}, {});
+  }
+
+  std::vector<Upstream::MockHostSet> host_sets_;
+  uint32_t update_frequency_{1};
+  NiceMock<Upstream::MockPrioritySet> priority_set_;
+  Upstream::RetryPrioritySharedPtr retry_priority_;
+};
+
+TEST_F(RetryPriorityTest, DefaultFrequency) {
+  initialize();
+
+  const Upstream::PriorityLoad original_priority_load{100, 0};
+  addHosts(0, 2, 2);
+  addHosts(1, 2, 2);
+
+  auto host1 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host1, priority()).WillByDefault(Return(0));
+
+  auto host2 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host2, priority()).WillByDefault(Return(1));
+
+  // Before any hosts attempted, load should be unchanged.
+  ASSERT_EQ(original_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+
+  const Upstream::PriorityLoad expected_priority_load{0, 100};
+  // After attempting a host in P0, P1 should receive all the load.
+  retry_priority_->onHostAttempted(host1);
+  ASSERT_EQ(expected_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+
+  // After we've tried host2, we've attempted all priorities and should reset back to the original
+  // priority load.
+  retry_priority_->onHostAttempted(host2);
+  ASSERT_EQ(original_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+}
+
+// Tests that spillover happens as we ignore attempted priorities.
+TEST_F(RetryPriorityTest, DefaultFrequencyDegradedPriorities) {
+  initialize();
+
+  const Upstream::PriorityLoad original_priority_load{42, 28, 30};
+  addHosts(0, 10, 3);
+  addHosts(1, 10, 2);
+  addHosts(2, 10, 10);
+
+  auto host1 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host1, priority()).WillByDefault(Return(0));
+
+  auto host2 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host2, priority()).WillByDefault(Return(1));
+
+  auto host3 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host3, priority()).WillByDefault(Return(2));
+
+  // Before any hosts attempted, load should be unchanged.
+  ASSERT_EQ(original_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+
+  {
+    // After attempting a host in P0, load should be split between P1 and P2 since P1 is degraded.
+    const Upstream::PriorityLoad expected_priority_load{0, 28, 72};
+    retry_priority_->onHostAttempted(host1);
+    ASSERT_EQ(expected_priority_load,
+              retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+  }
+
+  // After we've tried host2, everything should go to P2.
+  const Upstream::PriorityLoad expected_priority_load{0, 0, 100};
+  retry_priority_->onHostAttempted(host2);
+  ASSERT_EQ(expected_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+
+  // Once we've exhausted all priorities, we should return to the originial load.
+  retry_priority_->onHostAttempted(host3);
+  ASSERT_EQ(original_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+}
+
+// Tests that we can override the frequency at which we update the priority load with the
+// update_frequency parameter.
+TEST_F(RetryPriorityTest, OverridenFrequency) {
+  update_frequency_ = 2;
+  initialize();
+
+  const Upstream::PriorityLoad original_priority_load{100, 0};
+  addHosts(0, 2, 2);
+  addHosts(1, 2, 2);
+
+  auto host1 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host1, priority()).WillByDefault(Return(0));
+
+  auto host2 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host2, priority()).WillByDefault(Return(1));
+
+  // Before any hosts attempted, load should be unchanged.
+  ASSERT_EQ(original_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+
+  // After attempting a single host in P0, we should leave the priority load unchanged.
+  retry_priority_->onHostAttempted(host1);
+  ASSERT_EQ(original_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+
+  // After a second attempt, the prioity load should change.
+  const Upstream::PriorityLoad expected_priority_load{0, 100};
+  retry_priority_->onHostAttempted(host1);
+  ASSERT_EQ(expected_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+}
+
+// Verifies that the RetryPriority picks up on changes made to the cluster membership, adjusing
+// priority load appropriately.
+TEST_F(RetryPriorityTest, MembershipChange) {
+  initialize();
+
+  const Upstream::PriorityLoad original_priority_load{100, 0, 0};
+  addHosts(0, 2, 2);
+  addHosts(1, 2, 2);
+  addHosts(2, 2, 2);
+
+  auto host1 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host1, priority()).WillByDefault(Return(0));
+
+  auto host2 = std::make_shared<NiceMock<Upstream::MockHost>>();
+  ON_CALL(*host2, priority()).WillByDefault(Return(1));
+
+  // Before any hosts attempted, load should be unchanged.
+  ASSERT_EQ(original_priority_load,
+            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+
+  {
+    const Upstream::PriorityLoad expected_priority_load{0, 100, 0};
+    // After attempting a host in P0, P1 should receive all the load.
+    retry_priority_->onHostAttempted(host1);
+    ASSERT_EQ(expected_priority_load,
+              retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+  }
+
+  // Now update P1 to have 50% healthy hosts.
+  addHosts(1, 4, 2);
+
+  {
+    const Upstream::PriorityLoad expected_priority_load{0, 70, 30};
+    // After attempting a host in P0, P1 should receive all the load.
+    ASSERT_EQ(expected_priority_load,
+              retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
+  }
+}
+
+} // namespace Priority
+} // namespace Retry
+} // namespace Extensions
+} // namespace Envoy
diff --git a/test/integration/test_host_predicate_config.h b/test/integration/test_host_predicate_config.h
index 7e8c6f5c21d18..4835f29d43e96 100644
--- a/test/integration/test_host_predicate_config.h
+++ b/test/integration/test_host_predicate_config.h
@@ -13,5 +13,8 @@ class TestHostPredicateFactory : public Upstream::RetryHostPredicateFactory {
                            const Protobuf::Message&) override {
     callbacks.addHostPredicate(std::make_shared<TestHostPredicate>());
   }
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()};
+  }
 };
 } // namespace Envoy
diff --git a/test/mocks/upstream/mocks.h b/test/mocks/upstream/mocks.h
index 94b03fd0dfbf9..0fb4edae97a7f 100644
--- a/test/mocks/upstream/mocks.h
+++ b/test/mocks/upstream/mocks.h
@@ -124,6 +124,9 @@ class MockRetryPriorityFactory : public RetryPriorityFactory {
   }
 
   std::string name() const override { return "envoy.mock_retry_priority"; }
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()};
+  }
 
 private:
   RetryPrioritySharedPtr retry_priority_;

From f52c9dc53657b085da35b2b1e34cf4b9092c53db Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 25 Sep 2018 12:15:53 -0700
Subject: [PATCH 02/13] format

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../extensions/retry/priority/other_priority/other_priority.cc  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source/extensions/retry/priority/other_priority/other_priority.cc b/source/extensions/retry/priority/other_priority/other_priority.cc
index 9fa55beb114ec..a2cf4329d8873 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.cc
+++ b/source/extensions/retry/priority/other_priority/other_priority.cc
@@ -16,7 +16,7 @@ const Upstream::PriorityLoad& OtherPriorityRetryPriority::determinePriorityLoad(
     for (auto& host_set : priority_set_->hostSetsPerPriority()) {
       recalculatePerPriorityState(host_set->priority());
     }
-    callback_ = priority_set.addMemberUpdateCb([&,this](int priority, auto, auto) {
+    callback_ = priority_set.addMemberUpdateCb([&, this](int priority, auto, auto) {
       recalculatePerPriorityState(priority);
       adjustForAttemptedPriorities();
     });

From 6b6c02530b74d62ef8a7b8edaf331d5d60482716 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 25 Sep 2018 14:02:59 -0700
Subject: [PATCH 03/13] Kick CI

Signed-off-by: Snow Pettersen <snowp@squareup.com>

From 15967bc41ba42c4c93e433e3d062a5da27004788 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 25 Sep 2018 14:15:27 -0700
Subject: [PATCH 04/13] explicit this in lambda

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../retry/priority/other_priority/other_priority.cc           | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/source/extensions/retry/priority/other_priority/other_priority.cc b/source/extensions/retry/priority/other_priority/other_priority.cc
index a2cf4329d8873..f101abbaad36b 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.cc
+++ b/source/extensions/retry/priority/other_priority/other_priority.cc
@@ -17,8 +17,8 @@ const Upstream::PriorityLoad& OtherPriorityRetryPriority::determinePriorityLoad(
       recalculatePerPriorityState(host_set->priority());
     }
     callback_ = priority_set.addMemberUpdateCb([&, this](int priority, auto, auto) {
-      recalculatePerPriorityState(priority);
-      adjustForAttemptedPriorities();
+      this->recalculatePerPriorityState(priority);
+      this->adjustForAttemptedPriorities();
     });
   }
 

From 2274a5a7795b1061b395c4020c019fa8c99b915f Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 10:33:06 -0400
Subject: [PATCH 05/13] feedback: remove membership watch, other nits

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../other_priority_config.proto               |  4 ++
 source/common/upstream/load_balancer_impl.h   |  5 ++-
 .../priority/other_priority/other_priority.cc | 22 ++++-------
 .../priority/other_priority/other_priority.h  | 16 +++-----
 .../retry/priority/well_known_names.h         |  2 +-
 .../priority/other_priority/config_test.cc    | 39 -------------------
 6 files changed, 20 insertions(+), 68 deletions(-)

diff --git a/api/envoy/config/retry/other_priority/other_priority_config.proto b/api/envoy/config/retry/other_priority/other_priority_config.proto
index d7e41f9b52bf1..5a81866e9c3db 100644
--- a/api/envoy/config/retry/other_priority/other_priority_config.proto
+++ b/api/envoy/config/retry/other_priority/other_priority_config.proto
@@ -4,6 +4,10 @@ package envoy.config.retry.other_priority;
 
 // A retry host selector that attempts to spread retries between priorities, even if certain
 // priorities would not normally be attempted due to higher priorities being available.
+//
+// Note that changes made to the cluster during retries will not be reflected in the priority
+// load of retries, so care should be taken when using this with long running requests that
+// might retry.
 message OtherPriorityConfig {
   // How often the priority load should be updated based on previously attempted priorities. Useful
   // to allow each priorities to receive more than one request before being excluded.
diff --git a/source/common/upstream/load_balancer_impl.h b/source/common/upstream/load_balancer_impl.h
index 5b0baff811af2..83ea46401add1 100644
--- a/source/common/upstream/load_balancer_impl.h
+++ b/source/common/upstream/load_balancer_impl.h
@@ -72,12 +72,13 @@ class LoadBalancerBase : public LoadBalancer {
 
 public:
   // Called when a host set at the given priority level is updated. This updates
-  // per_priority_health_ for that priority level, and may update per_priority_load_ for all
+  // per_priority_health for that priority level, and may update per_priority_load for all
   // priority levels.
   void static recalculatePerPriorityState(uint32_t priority, const PrioritySet& priority_set,
                                           PriorityLoad& priority_load,
-                                          std::vector<uint32_t>& per_priority_health_);
+                                          std::vector<uint32_t>& per_priority_health);
 
+private:
   // The percentage load (0-100) for each priority level
   std::vector<uint32_t> per_priority_load_;
   // The health (0-100) for each priority level.
diff --git a/source/extensions/retry/priority/other_priority/other_priority.cc b/source/extensions/retry/priority/other_priority/other_priority.cc
index f101abbaad36b..18e5982043add 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.cc
+++ b/source/extensions/retry/priority/other_priority/other_priority.cc
@@ -7,19 +7,15 @@ namespace Priority {
 const Upstream::PriorityLoad& OtherPriorityRetryPriority::determinePriorityLoad(
     const Upstream::PrioritySet& priority_set,
     const Upstream::PriorityLoad& original_priority_load) {
-  if (!priority_set_) {
-    priority_set_ = &priority_set;
+  if (!initialized_) {
+    initialized_ = true;
 
     excluded_priorities_.resize(original_priority_load.size());
     // Initialize our local priority_load_ and priority_health_,
     // keeping them in sync with the member update cb.
-    for (auto& host_set : priority_set_->hostSetsPerPriority()) {
-      recalculatePerPriorityState(host_set->priority());
+    for (auto& host_set : priority_set.hostSetsPerPriority()) {
+      recalculatePerPriorityState(host_set->priority(), priority_set);
     }
-    callback_ = priority_set.addMemberUpdateCb([&, this](int priority, auto, auto) {
-      this->recalculatePerPriorityState(priority);
-      this->adjustForAttemptedPriorities();
-    });
   }
 
   // If we've not seen enough retries to modify the priority load, just
@@ -29,7 +25,7 @@ const Upstream::PriorityLoad& OtherPriorityRetryPriority::determinePriorityLoad(
   if (attempted_priorites_.size() < update_frequency_) {
     return original_priority_load;
   } else if (attempted_priorites_.size() % update_frequency_ == 0) {
-    for (auto priority : attempted_priorites_) {
+    for (const auto priority : attempted_priorites_) {
       excluded_priorities_[priority] = true;
     }
 
@@ -72,10 +68,8 @@ void OtherPriorityRetryPriority::adjustForAttemptedPriorities() {
     return;
   }
 
+  std::fill(per_priority_load_.begin(), per_priority_load_.end(), 0);
   // We then adjust the load by rebalancing priorities with the adjusted health values.
-  Upstream::PriorityLoad per_priority_load;
-  per_priority_load.resize(per_priority_load_.size());
-
   size_t total_load = 100;
   // The outer loop is used to eliminate rounding errors: any remaining load will be assigned to the
   // first healthy priority.
@@ -85,12 +79,10 @@ void OtherPriorityRetryPriority::adjustForAttemptedPriorities() {
       // when total_load runs out.
       auto delta =
           std::min<uint32_t>(total_load, adjusted_per_priority_health[i] * 100 / total_health);
-      per_priority_load[i] += delta;
+      per_priority_load_[i] += delta;
       total_load -= delta;
     }
   }
-
-  per_priority_load_ = per_priority_load;
 }
 } // namespace Priority
 } // namespace Retry
diff --git a/source/extensions/retry/priority/other_priority/other_priority.h b/source/extensions/retry/priority/other_priority/other_priority.h
index a55d583bfcdae..9951a941195bb 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.h
+++ b/source/extensions/retry/priority/other_priority/other_priority.h
@@ -12,11 +12,6 @@ namespace Priority {
 class OtherPriorityRetryPriority : public Upstream::RetryPriority {
 public:
   OtherPriorityRetryPriority(uint32_t update_frequency) : update_frequency_(update_frequency) {}
-  ~OtherPriorityRetryPriority() {
-    if (callback_) {
-      callback_->remove();
-    }
-  }
 
   const Upstream::PriorityLoad&
   determinePriorityLoad(const Upstream::PrioritySet& priority_set,
@@ -27,23 +22,22 @@ class OtherPriorityRetryPriority : public Upstream::RetryPriority {
   }
 
 private:
-  void recalculatePerPriorityState(uint32_t priority) {
+  void recalculatePerPriorityState(uint32_t priority, const Upstream::PrioritySet& priority_set) {
     // Recalcuate health and priority the same way the load balancer does it.
     Upstream::LoadBalancerBase::recalculatePerPriorityState(
-        priority, *priority_set_, per_priority_load_, per_priority_health_);
+        priority, priority_set, per_priority_load_, per_priority_health_);
   }
 
-  // Distributes priority load between priorities that should be consider after
+  // Distributes priority load between priorities that should be considered after
   // excluding attempted priorities.
   void adjustForAttemptedPriorities();
 
-  uint32_t update_frequency_;
+  const uint32_t update_frequency_;
   std::vector<uint32_t> attempted_priorites_;
   std::vector<bool> excluded_priorities_;
   Upstream::PriorityLoad per_priority_load_;
-  Upstream::PrioritySet const* priority_set_{};
-  Common::CallbackHandle* callback_{};
   std::vector<uint32_t> per_priority_health_;
+  bool initialized_{};
 };
 
 } // namespace Priority
diff --git a/source/extensions/retry/priority/well_known_names.h b/source/extensions/retry/priority/well_known_names.h
index ca99636f2962c..70989080e450c 100644
--- a/source/extensions/retry/priority/well_known_names.h
+++ b/source/extensions/retry/priority/well_known_names.h
@@ -12,7 +12,7 @@ namespace Priority {
  */
 class RetryPriorityNameValues {
 public:
-  // Previous host predicate. Rejects hosts that have already been tried.
+  // Previous priority retry priority. Excludes previously attempted priorities during retries.
   const std::string PreviousPrioritiesRetryPriority = "envoy.retry_priorities.previous_priorities";
 };
 
diff --git a/test/extensions/retry/priority/other_priority/config_test.cc b/test/extensions/retry/priority/other_priority/config_test.cc
index 475c28be0f9c3..a3dbe8080fd9e 100644
--- a/test/extensions/retry/priority/other_priority/config_test.cc
+++ b/test/extensions/retry/priority/other_priority/config_test.cc
@@ -151,45 +151,6 @@ TEST_F(RetryPriorityTest, OverridenFrequency) {
             retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
 }
 
-// Verifies that the RetryPriority picks up on changes made to the cluster membership, adjusing
-// priority load appropriately.
-TEST_F(RetryPriorityTest, MembershipChange) {
-  initialize();
-
-  const Upstream::PriorityLoad original_priority_load{100, 0, 0};
-  addHosts(0, 2, 2);
-  addHosts(1, 2, 2);
-  addHosts(2, 2, 2);
-
-  auto host1 = std::make_shared<NiceMock<Upstream::MockHost>>();
-  ON_CALL(*host1, priority()).WillByDefault(Return(0));
-
-  auto host2 = std::make_shared<NiceMock<Upstream::MockHost>>();
-  ON_CALL(*host2, priority()).WillByDefault(Return(1));
-
-  // Before any hosts attempted, load should be unchanged.
-  ASSERT_EQ(original_priority_load,
-            retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
-
-  {
-    const Upstream::PriorityLoad expected_priority_load{0, 100, 0};
-    // After attempting a host in P0, P1 should receive all the load.
-    retry_priority_->onHostAttempted(host1);
-    ASSERT_EQ(expected_priority_load,
-              retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
-  }
-
-  // Now update P1 to have 50% healthy hosts.
-  addHosts(1, 4, 2);
-
-  {
-    const Upstream::PriorityLoad expected_priority_load{0, 70, 30};
-    // After attempting a host in P0, P1 should receive all the load.
-    ASSERT_EQ(expected_priority_load,
-              retry_priority_->determinePriorityLoad(priority_set_, original_priority_load));
-  }
-}
-
 } // namespace Priority
 } // namespace Retry
 } // namespace Extensions

From 158d08440d9b84552ca796f11b6cdba15fd11108 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 10:37:32 -0400
Subject: [PATCH 06/13] add example to docs

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../config/retry/other_priority/other_priority_config.proto    | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/api/envoy/config/retry/other_priority/other_priority_config.proto b/api/envoy/config/retry/other_priority/other_priority_config.proto
index 5a81866e9c3db..f38e962cca95f 100644
--- a/api/envoy/config/retry/other_priority/other_priority_config.proto
+++ b/api/envoy/config/retry/other_priority/other_priority_config.proto
@@ -11,5 +11,8 @@ package envoy.config.retry.other_priority;
 message OtherPriorityConfig {
   // How often the priority load should be updated based on previously attempted priorities. Useful
   // to allow each priorities to receive more than one request before being excluded.
+  // For example, by setting this to 2, then the first two attempts (initial attempt and one retry)
+  // will use the unmodified priority load. The third and fourth attempt will use priority load which
+  // excludes the priorities routed to with the first two attempts.
   int32 update_frequency = 1;
 }

From f61969660ae20f563605956c1926483e76c033dd Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 11:05:02 -0400
Subject: [PATCH 07/13] private -> protected

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 source/common/upstream/load_balancer_impl.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source/common/upstream/load_balancer_impl.h b/source/common/upstream/load_balancer_impl.h
index 83ea46401add1..2c8fa28f81e88 100644
--- a/source/common/upstream/load_balancer_impl.h
+++ b/source/common/upstream/load_balancer_impl.h
@@ -78,7 +78,7 @@ class LoadBalancerBase : public LoadBalancer {
                                           PriorityLoad& priority_load,
                                           std::vector<uint32_t>& per_priority_health);
 
-private:
+protected:
   // The percentage load (0-100) for each priority level
   std::vector<uint32_t> per_priority_load_;
   // The health (0-100) for each priority level.

From 16be1ce7198da52684d14cdb5efa233ca79df829 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 11:12:00 -0400
Subject: [PATCH 08/13] format

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../config/retry/other_priority/other_priority_config.proto   | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/api/envoy/config/retry/other_priority/other_priority_config.proto b/api/envoy/config/retry/other_priority/other_priority_config.proto
index f38e962cca95f..939da0b0dc3c4 100644
--- a/api/envoy/config/retry/other_priority/other_priority_config.proto
+++ b/api/envoy/config/retry/other_priority/other_priority_config.proto
@@ -12,7 +12,7 @@ message OtherPriorityConfig {
   // How often the priority load should be updated based on previously attempted priorities. Useful
   // to allow each priorities to receive more than one request before being excluded.
   // For example, by setting this to 2, then the first two attempts (initial attempt and one retry)
-  // will use the unmodified priority load. The third and fourth attempt will use priority load which
-  // excludes the priorities routed to with the first two attempts.
+  // will use the unmodified priority load. The third and fourth attempt will use priority load
+  // which excludes the priorities routed to with the first two attempts.
   int32 update_frequency = 1;
 }

From 60c4fa6dfb8c2caeb5c03dd6c676a224ecfa4378 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 10:15:02 -0700
Subject: [PATCH 09/13] update comments, rebuild entire priority_load on update

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../other_priority_config.proto               | 26 +++++--
 .../priority/other_priority/other_priority.cc | 75 ++++++++++---------
 .../priority/other_priority/other_priority.h  |  8 +-
 3 files changed, 63 insertions(+), 46 deletions(-)

diff --git a/api/envoy/config/retry/other_priority/other_priority_config.proto b/api/envoy/config/retry/other_priority/other_priority_config.proto
index 939da0b0dc3c4..0b6bc995bca82 100644
--- a/api/envoy/config/retry/other_priority/other_priority_config.proto
+++ b/api/envoy/config/retry/other_priority/other_priority_config.proto
@@ -5,14 +5,26 @@ package envoy.config.retry.other_priority;
 // A retry host selector that attempts to spread retries between priorities, even if certain
 // priorities would not normally be attempted due to higher priorities being available.
 //
-// Note that changes made to the cluster during retries will not be reflected in the priority
-// load of retries, so care should be taken when using this with long running requests that
-// might retry.
+// Each priority attempted will be excluded until there are no healthy priorities left, at which
+// point the list of attempted priorities will be reset, essentially starting from the beginning.
+// For example, given three priorities P0, P1, P2 with healthy % of 100, 0 and 50 respectively, the
+// following sequence of priorities would be selected (assuming update_frequency = 1):
+// Attempt 1: P0 (P0 is 100% healthy)
+// Attempt 2: P2 (P0 already attempted, P2 only healthy priority)
+// Attempt 3: P0 (no healthy priorities, reset)
+// Attempt 4: P2
+//
+// Using this PriorityFilter requires rebuilding the priority load, which runs in O(# of
+// priorities), which might incur significant overhead for clusters with many priorities.
 message OtherPriorityConfig {
   // How often the priority load should be updated based on previously attempted priorities. Useful
-  // to allow each priorities to receive more than one request before being excluded.
-  // For example, by setting this to 2, then the first two attempts (initial attempt and one retry)
-  // will use the unmodified priority load. The third and fourth attempt will use priority load
-  // which excludes the priorities routed to with the first two attempts.
+  // to allow each priorities to receive more than one request before being excluded or to reduce
+  // the number of times that the priority load has to be recomputed.
+  //
+  // For example, by setting this to 2, then the first two attempts (initial attempt and first
+  // retry) will use the unmodified priority load. The third and fourth attempt will use priority
+  // load which excludes the priorities routed to with the first two attempts, and the fifth and
+  // sixth attempt will use the priority load excluding the priorities used for the first four
+  // attempts.
   int32 update_frequency = 1;
 }
diff --git a/source/extensions/retry/priority/other_priority/other_priority.cc b/source/extensions/retry/priority/other_priority/other_priority.cc
index 18e5982043add..4b65cbd2f3f51 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.cc
+++ b/source/extensions/retry/priority/other_priority/other_priority.cc
@@ -7,67 +7,54 @@ namespace Priority {
 const Upstream::PriorityLoad& OtherPriorityRetryPriority::determinePriorityLoad(
     const Upstream::PrioritySet& priority_set,
     const Upstream::PriorityLoad& original_priority_load) {
-  if (!initialized_) {
-    initialized_ = true;
-
-    excluded_priorities_.resize(original_priority_load.size());
-    // Initialize our local priority_load_ and priority_health_,
-    // keeping them in sync with the member update cb.
-    for (auto& host_set : priority_set.hostSetsPerPriority()) {
-      recalculatePerPriorityState(host_set->priority(), priority_set);
-    }
-  }
-
   // If we've not seen enough retries to modify the priority load, just
   // return the original.
   // If this retry should trigger an update, recalculate the priority load by excluding attempted
   // priorities.
-  if (attempted_priorites_.size() < update_frequency_) {
+  if (attempted_priorities_.size() < update_frequency_) {
     return original_priority_load;
-  } else if (attempted_priorites_.size() % update_frequency_ == 0) {
-    for (const auto priority : attempted_priorites_) {
+  } else if (attempted_priorities_.size() % update_frequency_ == 0) {
+    if (excluded_priorities_.size() < priority_set.hostSetsPerPriority().size()) {
+      excluded_priorities_.resize(priority_set.hostSetsPerPriority().size());
+    }
+
+    for (const auto priority : attempted_priorities_) {
       excluded_priorities_[priority] = true;
     }
 
-    adjustForAttemptedPriorities();
+    adjustForAttemptedPriorities(priority_set);
   }
 
   return per_priority_load_;
 }
 
-void OtherPriorityRetryPriority::adjustForAttemptedPriorities() {
+void OtherPriorityRetryPriority::adjustForAttemptedPriorities(
+    const Upstream::PrioritySet& priority_set) {
+  for (auto& host_set : priority_set.hostSetsPerPriority()) {
+    recalculatePerPriorityState(host_set->priority(), priority_set);
+  }
+
   // If all priorities are unhealthy to begin with, there's nothing to do.
   if (!std::accumulate(per_priority_load_.begin(), per_priority_load_.end(), 0)) {
     return;
   }
 
-  // First we create an adjusted health view of the priorities, where attempted priorities are
-  // given a zero weight.
-  uint32_t total_health = 0;
-  std::vector<uint32_t> adjusted_per_priority_health;
-  adjusted_per_priority_health.resize(per_priority_health_.size());
-  for (size_t i = 0; i < per_priority_health_.size(); ++i) {
-    if (!excluded_priorities_[i]) {
-      adjusted_per_priority_health[i] = per_priority_health_[i];
-      total_health += per_priority_health_[i];
-    }
-  }
-
-  total_health = std::min(total_health, 100u);
-
-  // If there are no healthy priorities left, we reset the attempted priorities and recurse.
+  auto adjustedHealthAndSum = adjustedHealth();
+  // If there are no healthy priorities left, we reset the attempted priorities and recompute the
+  // adjusted health.
   // This allows us to fall back to the unmodified priority load when we run out of priorites
   // instead of failing to route requests.
-  if (total_health == 0) {
+  if (adjustedHealthAndSum.second == 0) {
     for (size_t i = 0; i < excluded_priorities_.size(); ++i) {
       excluded_priorities_[i] = false;
     }
-    attempted_priorites_.clear();
-
-    adjustForAttemptedPriorities();
-    return;
+    attempted_priorities_.clear();
+    adjustedHealthAndSum = adjustedHealth();
   }
 
+  const auto& adjusted_per_priority_health = adjustedHealthAndSum.first;
+  auto total_health = adjustedHealthAndSum.second;
+
   std::fill(per_priority_load_.begin(), per_priority_load_.end(), 0);
   // We then adjust the load by rebalancing priorities with the adjusted health values.
   size_t total_load = 100;
@@ -84,6 +71,22 @@ void OtherPriorityRetryPriority::adjustForAttemptedPriorities() {
     }
   }
 }
+
+std::pair<std::vector<uint32_t>, uint32_t> OtherPriorityRetryPriority::adjustedHealth() const {
+  uint32_t total_health = 0;
+  std::vector<uint32_t> adjusted_per_priority_health(per_priority_health_.size());
+  adjusted_per_priority_health.resize(per_priority_health_.size());
+
+  for (size_t i = 0; i < per_priority_health_.size(); ++i) {
+    if (!excluded_priorities_[i]) {
+      adjusted_per_priority_health[i] = per_priority_health_[i];
+      total_health += per_priority_health_[i];
+    }
+  }
+
+  return {std::move(adjusted_per_priority_health), std::min(total_health, 100u)};
+}
+
 } // namespace Priority
 } // namespace Retry
 } // namespace Extensions
diff --git a/source/extensions/retry/priority/other_priority/other_priority.h b/source/extensions/retry/priority/other_priority/other_priority.h
index 9951a941195bb..0cc17f9191b5c 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.h
+++ b/source/extensions/retry/priority/other_priority/other_priority.h
@@ -18,7 +18,7 @@ class OtherPriorityRetryPriority : public Upstream::RetryPriority {
                         const Upstream::PriorityLoad& original_priority) override;
 
   void onHostAttempted(Upstream::HostDescriptionConstSharedPtr attempted_host) override {
-    attempted_priorites_.emplace_back(attempted_host->priority());
+    attempted_priorities_.emplace_back(attempted_host->priority());
   }
 
 private:
@@ -28,12 +28,14 @@ class OtherPriorityRetryPriority : public Upstream::RetryPriority {
         priority, priority_set, per_priority_load_, per_priority_health_);
   }
 
+  std::pair<std::vector<uint32_t>, uint32_t> adjustedHealth() const;
+
   // Distributes priority load between priorities that should be considered after
   // excluding attempted priorities.
-  void adjustForAttemptedPriorities();
+  void adjustForAttemptedPriorities(const Upstream::PrioritySet& priority_set);
 
   const uint32_t update_frequency_;
-  std::vector<uint32_t> attempted_priorites_;
+  std::vector<uint32_t> attempted_priorities_;
   std::vector<bool> excluded_priorities_;
   Upstream::PriorityLoad per_priority_load_;
   std::vector<uint32_t> per_priority_health_;

From 6d446f5d90a22de365e55a54381c6325ad01cb95 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 10:30:01 -0700
Subject: [PATCH 10/13] add comment on how load is distributed when priorities
 are excluded

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../retry/other_priority/other_priority_config.proto       | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/api/envoy/config/retry/other_priority/other_priority_config.proto b/api/envoy/config/retry/other_priority/other_priority_config.proto
index 0b6bc995bca82..0113417619ddb 100644
--- a/api/envoy/config/retry/other_priority/other_priority_config.proto
+++ b/api/envoy/config/retry/other_priority/other_priority_config.proto
@@ -5,6 +5,13 @@ package envoy.config.retry.other_priority;
 // A retry host selector that attempts to spread retries between priorities, even if certain
 // priorities would not normally be attempted due to higher priorities being available.
 //
+// As priorities get excluded, load will be distributed amongst the remaining healthy priorities
+// based on the relative health of the priorities, matching how load is distributed during regular
+// host selection. For example, given priority healths of {100, 50, 50}, the original load will be
+// {100, 0, 0} (since P0 has capacity to handle 100% of the traffic). If P0 is excluded, the load
+// changes to {0, 50, 50}, because P1 is only able to handle 50% of the traffic, causing the
+// remaining to spill over to P2.
+//
 // Each priority attempted will be excluded until there are no healthy priorities left, at which
 // point the list of attempted priorities will be reset, essentially starting from the beginning.
 // For example, given three priorities P0, P1, P2 with healthy % of 100, 0 and 50 respectively, the

From e5b3b36876639bbcfee614a475886aeaa643d8f9 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 10:55:24 -0700
Subject: [PATCH 11/13] reserve instead of fill

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../retry/priority/other_priority/other_priority.h           | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/source/extensions/retry/priority/other_priority/other_priority.h b/source/extensions/retry/priority/other_priority/other_priority.h
index 38d2e0caebf7f..b8fcd8e9e12e0 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.h
+++ b/source/extensions/retry/priority/other_priority/other_priority.h
@@ -11,7 +11,10 @@ namespace Priority {
 
 class OtherPriorityRetryPriority : public Upstream::RetryPriority {
 public:
-  OtherPriorityRetryPriority(uint32_t update_frequency, uint32_t max_retries) : update_frequency_(update_frequency), attempted_priorities_(max_retries) {}
+  OtherPriorityRetryPriority(uint32_t update_frequency, uint32_t max_retries)
+      : update_frequency_(update_frequency) {
+    attempted_priorities_.reserve(max_retries);
+  }
 
   const Upstream::PriorityLoad&
   determinePriorityLoad(const Upstream::PrioritySet& priority_set,

From 48c09638836d35b6bc358d8d08319e328cfe8ed3 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Tue, 2 Oct 2018 11:07:04 -0700
Subject: [PATCH 12/13] format

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 source/extensions/retry/priority/other_priority/config.cc | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/source/extensions/retry/priority/other_priority/config.cc b/source/extensions/retry/priority/other_priority/config.cc
index f52c213b4c61e..345e482b048ab 100644
--- a/source/extensions/retry/priority/other_priority/config.cc
+++ b/source/extensions/retry/priority/other_priority/config.cc
@@ -10,11 +10,13 @@ namespace Retry {
 namespace Priority {
 
 void OtherPriorityRetryPriorityFactory::createRetryPriority(
-    Upstream::RetryPriorityFactoryCallbacks& callbacks, const Protobuf::Message& config, uint32_t max_retries) {
+    Upstream::RetryPriorityFactoryCallbacks& callbacks, const Protobuf::Message& config,
+    uint32_t max_retries) {
   callbacks.addRetryPriority(std::make_shared<OtherPriorityRetryPriority>(
       MessageUtil::downcastAndValidate<
           const envoy::config::retry::other_priority::OtherPriorityConfig&>(config)
-          .update_frequency(), max_retries));
+          .update_frequency(),
+      max_retries));
 }
 
 static Registry::RegisterFactory<OtherPriorityRetryPriorityFactory, Upstream::RetryPriorityFactory>

From b8abcd3da38593829ef80bbc0b773dafeb162699 Mon Sep 17 00:00:00 2001
From: Snow Pettersen <snowp@squareup.com>
Date: Thu, 4 Oct 2018 07:03:31 -0700
Subject: [PATCH 13/13] add back comment

Signed-off-by: Snow Pettersen <snowp@squareup.com>
---
 .../extensions/retry/priority/other_priority/other_priority.cc  | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/source/extensions/retry/priority/other_priority/other_priority.cc b/source/extensions/retry/priority/other_priority/other_priority.cc
index 4b65cbd2f3f51..1d6cf7a6cebe5 100644
--- a/source/extensions/retry/priority/other_priority/other_priority.cc
+++ b/source/extensions/retry/priority/other_priority/other_priority.cc
@@ -73,6 +73,8 @@ void OtherPriorityRetryPriority::adjustForAttemptedPriorities(
 }
 
 std::pair<std::vector<uint32_t>, uint32_t> OtherPriorityRetryPriority::adjustedHealth() const {
+  // Create an adjusted health view of the priorities, where attempted priorities are
+  // given a zero weight.
   uint32_t total_health = 0;
   std::vector<uint32_t> adjusted_per_priority_health(per_priority_health_.size());
   adjusted_per_priority_health.resize(per_priority_health_.size());