-
Notifications
You must be signed in to change notification settings - Fork 5.5k
upstream: partition hosts in a single pass #6440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
4ffd117
cf32ab4
9435169
5de20f4
9032ed7
44d7231
2b55e31
90a2c86
8f285c5
89842a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -272,21 +272,42 @@ void HostImpl::weight(uint32_t new_weight) { weight_ = std::max(1U, std::min(128 | |
|
|
||
| HostsPerLocalityConstSharedPtr | ||
| HostsPerLocalityImpl::filter(std::function<bool(const Host&)> predicate) const { | ||
| auto* filtered_clone = new HostsPerLocalityImpl(); | ||
| HostsPerLocalityConstSharedPtr shared_filtered_clone{filtered_clone}; | ||
| return filter(std::vector<std::function<bool(const Host&)>>{predicate})[0]; | ||
| } | ||
|
|
||
| std::vector<HostsPerLocalityConstSharedPtr> HostsPerLocalityImpl::filter( | ||
| const std::vector<std::function<bool(const Host&)>>& predicates) const { | ||
| // We keep two lists: one for being able to mutate the clone and one for returning to the caller. | ||
| // Creating them both at the start avoids iterating over the mutable values at the end to convert | ||
| // them to a const pointer. | ||
| std::vector<HostsPerLocalityImpl*> mutable_clones; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a little odd that we are using a raw pointer for this vector. Can we use a shared_ptr here also? I think a mutable shared pointer can be shared ptr casted to const so this should work but not 100% sure.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll give it a go |
||
| std::vector<HostsPerLocalityConstSharedPtr> filtered_clones; | ||
|
|
||
| for (size_t i = 0; i < predicates.size(); ++i) { | ||
| mutable_clones.emplace_back(new HostsPerLocalityImpl()); | ||
| filtered_clones.emplace_back(mutable_clones.back()); | ||
| mutable_clones.back()->local_ = local_; | ||
| } | ||
|
|
||
| filtered_clone->local_ = local_; | ||
| for (const auto& hosts_locality : hosts_per_locality_) { | ||
| HostVector current_locality_hosts; | ||
| std::vector<HostVector> current_locality_hosts; | ||
| current_locality_hosts.resize(predicates.size()); | ||
|
|
||
| // Since # of hosts >> # of predicates, we iterate over the hosts in the outer loop. | ||
| for (const auto& host : hosts_locality) { | ||
| if (predicate(*host)) { | ||
| current_locality_hosts.emplace_back(host); | ||
| for (size_t i = 0; i < predicates.size(); ++i) { | ||
| if (predicates[i](*host)) { | ||
| current_locality_hosts[i].emplace_back(host); | ||
| } | ||
| } | ||
| } | ||
| filtered_clone->hosts_per_locality_.push_back(std::move(current_locality_hosts)); | ||
|
|
||
| for (size_t i = 0; i < predicates.size(); ++i) { | ||
| mutable_clones[i]->hosts_per_locality_.push_back(std::move(current_locality_hosts[0])); | ||
| } | ||
| } | ||
|
|
||
| return shared_filtered_clone; | ||
| return filtered_clones; | ||
| } | ||
|
|
||
| void HostSetImpl::updateHosts(PrioritySet::UpdateHostsParams&& update_hosts_params, | ||
|
|
@@ -414,16 +435,15 @@ HostSetImpl::updateHostsParams(HostVectorConstSharedPtr hosts, | |
| PrioritySet::UpdateHostsParams | ||
| HostSetImpl::partitionHosts(HostVectorConstSharedPtr hosts, | ||
| HostsPerLocalityConstSharedPtr hosts_per_locality) { | ||
| auto healthy_hosts = ClusterImplBase::createHostList(*hosts, Host::Health::Healthy); | ||
| auto degraded_hosts = ClusterImplBase::createHostList(*hosts, Host::Health::Degraded); | ||
| auto healthy_hosts_per_locality = | ||
| ClusterImplBase::createHostLists(*hosts_per_locality, Host::Health::Healthy); | ||
| auto degraded_hosts_per_locality = | ||
| ClusterImplBase::createHostLists(*hosts_per_locality, Host::Health::Degraded); | ||
| auto healthy_and_degraded_hosts = ClusterImplBase::partitionHostList(*hosts); | ||
| auto healthy_and_degraded_hosts_per_locality = | ||
| ClusterImplBase::partitionHostsPerLocality(*hosts_per_locality); | ||
|
|
||
| return updateHostsParams(std::move(hosts), std::move(hosts_per_locality), | ||
| std::move(healthy_hosts), std::move(healthy_hosts_per_locality), | ||
| std::move(degraded_hosts), std::move(degraded_hosts_per_locality)); | ||
| std::move(healthy_and_degraded_hosts.first), | ||
| std::move(healthy_and_degraded_hosts_per_locality.first), | ||
| std::move(healthy_and_degraded_hosts.second), | ||
| std::move(healthy_and_degraded_hosts_per_locality.second)); | ||
| } | ||
|
|
||
| double HostSetImpl::effectiveLocalityWeight(uint32_t index, | ||
|
|
@@ -673,21 +693,30 @@ ClusterImplBase::ClusterImplBase( | |
| }); | ||
| } | ||
|
|
||
| HostVectorConstSharedPtr ClusterImplBase::createHostList(const HostVector& hosts, | ||
| Host::Health health) { | ||
| HostVectorSharedPtr healthy_list(new HostVector()); | ||
| std::pair<HostVectorConstSharedPtr, HostVectorConstSharedPtr> | ||
| ClusterImplBase::partitionHostList(const HostVector& hosts) { | ||
| auto healthy_list = std::make_shared<HostVector>(); | ||
| auto degraded_list = std::make_shared<HostVector>(); | ||
|
|
||
| for (const auto& host : hosts) { | ||
| if (host->health() == health) { | ||
| if (host->health() == Host::Health::Healthy) { | ||
| healthy_list->emplace_back(host); | ||
| } | ||
| if (host->health() == Host::Health::Degraded) { | ||
| degraded_list->emplace_back(host); | ||
| } | ||
| } | ||
|
|
||
| return healthy_list; | ||
| return {healthy_list, degraded_list}; | ||
| } | ||
|
|
||
| HostsPerLocalityConstSharedPtr ClusterImplBase::createHostLists(const HostsPerLocality& hosts, | ||
| Host::Health health) { | ||
| return hosts.filter([&health](const Host& host) { return host.health() == health; }); | ||
| std::pair<HostsPerLocalityConstSharedPtr, HostsPerLocalityConstSharedPtr> | ||
| ClusterImplBase::partitionHostsPerLocality(const HostsPerLocality& hosts) { | ||
| auto filtered_clones = | ||
| hosts.filter({[](const Host& host) { return host.health() == Host::Health::Healthy; }, | ||
| [](const Host& host) { return host.health() == Host::Health::Degraded; }}); | ||
|
|
||
| return {std::move(filtered_clones[0]), std::move(filtered_clones[1])}; | ||
| } | ||
|
|
||
| bool ClusterInfoImpl::maintenanceMode() const { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -257,6 +257,8 @@ class HostsPerLocalityImpl : public HostsPerLocality { | |
| bool hasLocalLocality() const override { return local_; } | ||
| const std::vector<HostVector>& get() const override { return hosts_per_locality_; } | ||
| HostsPerLocalityConstSharedPtr filter(std::function<bool(const Host&)> predicate) const override; | ||
| std::vector<HostsPerLocalityConstSharedPtr> | ||
| filter(const std::vector<std::function<bool(const Host&)>>& predicate) const override; | ||
|
|
||
| // The const shared pointer for the empty HostsPerLocalityImpl. | ||
| static HostsPerLocalityConstSharedPtr empty() { | ||
|
|
@@ -637,9 +639,14 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable<Logger::Id::u | |
| const Network::Address::InstanceConstSharedPtr | ||
| resolveProtoAddress(const envoy::api::v2::core::Address& address); | ||
|
|
||
| static HostVectorConstSharedPtr createHostList(const HostVector& hosts, Host::Health health); | ||
| static HostsPerLocalityConstSharedPtr createHostLists(const HostsPerLocality& hosts, | ||
| Host::Health); | ||
| // Partitions the provided list of hosts into two new lists containing the healthy and degraded | ||
| // hosts respectively. | ||
| static std::pair<HostVectorConstSharedPtr, HostVectorConstSharedPtr> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth doing the same type tricks you did previously to make it more clear that the first is healthy and the second is degraded? I think this might be worth it in the this case? |
||
| partitionHostList(const HostVector& hosts); | ||
| // Partitions the provided list of hosts per locality into two new lists containing the healthy | ||
| // and degraded hosts respectively. | ||
| static std::pair<HostsPerLocalityConstSharedPtr, HostsPerLocalityConstSharedPtr> | ||
| partitionHostsPerLocality(const HostsPerLocality& hosts); | ||
|
|
||
| // Upstream::Cluster | ||
| HealthChecker* healthChecker() override { return health_checker_.get(); } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just get rid of this variant and have callers use the vector version? It doesn't seem worth it to me to maintain both, but I don't feel strongly about it if you do. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i'll just remove this