fixes for Alyssa's review feedbacks: If eds config update not contain…

…ing a overprovisioning factor, assumes the default value Signed-off-by: Xin Zhuang <[email protected]>
envoyproxy · Aug 9, 2018 · aaf0a9b · aaf0a9b
1 parent 8631fca
commit aaf0a9b
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 12 deletions.
diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc
@@ -85,10 +85,8 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std::
   // Track whether we rebuilt any LB structures.
   bool cluster_rebuilt = false;
 
-  absl::optional<uint32_t> overprovisioning_factor = absl::nullopt;
-  if (cluster_load_assignment.policy().has_overprovisioning_factor()) {
-    overprovisioning_factor = cluster_load_assignment.policy().overprovisioning_factor().value();
-  }
+  const uint32_t overprovisioning_factor = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
+      cluster_load_assignment.policy(), overprovisioning_factor, kDefaultOverProvisioningFactor);
 
   // Loop over existing priorities not present in the config. This will empty out any priorities
   // the config update did not refer to
@@ -128,7 +126,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std::
 }
 
 bool EdsClusterImpl::updateHostsPerLocality(const uint32_t priority,
-                                            absl::optional<uint32_t> overprovisioning_factor,
+                                            const uint32_t overprovisioning_factor,
                                             const HostVector& new_hosts,
                                             LocalityWeightsMap& locality_weights_map,
                                             LocalityWeightsMap& new_locality_weights_map,
@@ -146,7 +144,7 @@ bool EdsClusterImpl::updateHostsPerLocality(const uint32_t priority,
   // out of the locality scheduler, we discover their new weights. We don't currently have a shared
   // object for locality weights that we can update here, we should add something like this to
   // improve performance and scalability of locality weight updates.
-  if (overprovisioning_factor.has_value() ||
+  if (host_set.overprovisioning_factor() != overprovisioning_factor ||
       updateDynamicHostList(new_hosts, *current_hosts_copy, hosts_added, hosts_removed) ||
       locality_weights_map != new_locality_weights_map) {
     locality_weights_map = new_locality_weights_map;

diff --git a/source/common/upstream/eds.h b/source/common/upstream/eds.h
@@ -36,8 +36,7 @@ class EdsClusterImpl : public BaseDynamicClusterImpl,
 private:
   using LocalityWeightsMap =
       std::unordered_map<envoy::api::v2::core::Locality, uint32_t, LocalityHash, LocalityEqualTo>;
-  bool updateHostsPerLocality(const uint32_t priority,
-                              absl::optional<uint32_t> overprovisioning_factor,
+  bool updateHostsPerLocality(const uint32_t priority, const uint32_t overprovisioning_factor,
                               const HostVector& new_hosts, LocalityWeightsMap& locality_weights_map,
                               LocalityWeightsMap& new_locality_weights_map,
                               PriorityStateManager& priority_state_manager);

diff --git a/source/common/upstream/subset_lb.cc b/source/common/upstream/subset_lb.cc
@@ -551,10 +551,8 @@ HostSetImplPtr SubsetLoadBalancer::PrioritySubsetImpl::createHostSet(
 
   const HostSetPtr& host_set = original_priority_set_.hostSetsPerPriority()[priority];
 
-  UNREFERENCED_PARAMETER(overprovisioning_factor);
-  if (overprovisioning_factor.has_value()) {
-    ASSERT(overprovisioning_factor.value() == host_set->overprovisioning_factor());
-  }
+  ASSERT(!overprovisioning_factor.has_value() ||
+         overprovisioning_factor.value() == host_set->overprovisioning_factor());
   return HostSetImplPtr{new HostSubsetImpl(*host_set, locality_weight_aware_)};
 }