Skip to content

Commit

Permalink
hash endpoints and health check lists to stop not needed updates
Browse files Browse the repository at this point in the history 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
  • Loading branch information
@drewsortega
drewsortega committed Sep 6, 2020
1 parent f32ac32 commit 20f069f
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 7 deletions.
33 changes: 26 additions & 7 deletions source/common/upstream/health_discovery_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,9 @@ HdsCluster::HdsCluster(Server::Admin& admin, Runtime::Loader& runtime,
validation_visitor_(validation_visitor) {
ENVOY_LOG(debug, "Creating an HdsCluster");
priority_set_.getOrCreateHostSet(0);
config_hash_ = MessageUtil::hash(cluster_);
endpoints_hash_ = RepeatedPtrUtil::hash(cluster_.load_assignment().endpoints());

This comment has been minimized.

Sign in to view

Copy link
@xdzhai

xdzhai Sep 7, 2020

Contributor

I feel hash comparison might be overused.
There are hash created for specifier proto, cluster_health_check, cluster config, cluster endpoints, each hashing need to process the whole proto.
We may discuss with Harvey later to get his suggestion.
health_checkers_hash_ = RepeatedPtrUtil::hash(cluster_.health_checks());

info_ = info_factory.createClusterInfo(
{admin, runtime_, cluster_, bind_config_, stats_, ssl_context_manager_, added_via_api_, cm,
Expand Down Expand Up @@ -402,14 +405,30 @@ void HdsCluster::update(Server::Admin& admin, envoy::config::cluster::v3::Cluste
Random::RandomGenerator& random, Singleton::Manager& singleton_manager,
ThreadLocal::SlotAllocator& tls,
ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) {
// TODO(drewsortega): skip this entire function body if the config hash is the same
cluster_ = std::move(cluster);
info_ = info_factory.createClusterInfo(
{admin, runtime_, cluster_, bind_config_, stats_, ssl_context_manager_, added_via_api_, cm,
local_info, dispatcher, random, singleton_manager, tls, validation_visitor, api});
const uint64_t config_hash = MessageUtil::hash(cluster);
// if this is a different config then what we already have, update the cluster.
if (config_hash_ != config_hash) {
config_hash_ = config_hash;
cluster_ = std::move(cluster);

// always update our info_
info_ = info_factory.createClusterInfo(
{admin, runtime_, cluster_, bind_config_, stats_, ssl_context_manager_, added_via_api_, cm,
local_info, dispatcher, random, singleton_manager, tls, validation_visitor, api});

const auto& endpoints = cluster.load_assignment().endpoints();
const uint64_t endpoints_hash = RepeatedPtrUtil::hash(endpoints);
if (endpoints_hash_ != endpoints_hash) {
endpoints_hash_ = endpoints_hash;
updateHosts(endpoints);
}

updateHosts(cluster_.load_assignment().endpoints());
updateHealthchecks(cluster_.health_checks());
const uint64_t health_checkers_hash = RepeatedPtrUtil::hash(cluster.health_checks());
if (health_checkers_hash_ != health_checkers_hash) {
health_checkers_hash_ = health_checkers_hash;
updateHealthchecks(cluster_.health_checks());

This comment has been minimized.

Sign in to view

Copy link
@xdzhai

xdzhai Sep 7, 2020 •
edited
Loading

Contributor

health_checks config should be rarely updated, usually user just update endpoints.
I feel it doesn't worth the complexity of handling health_checks changes, e.g. if a health_check config field is changed, you will need to remove the old health_check and add a new health_check, and add all hosts.
From my opinion, I would prefer to make it simple by just recreating the HdsCluster, no need to worry about optimization since it rarely happens.
You may consider adding a HdsCluster member function like:
bool configFieldsChanged(new_cluster_config), and recreate the whole HdsCluster if it returns true.
Currently maybe only health_checks() need to be compared in this function, in the future it may check more global fields which require a whole recreation.
To compare the repeated field, you can use std::equal with a predicate using proto2::util::MessageDifferencer::Equals().
}
}
}

void HdsCluster::updateHealthchecks(
Expand Down
3 changes: 3 additions & 0 deletions source/common/upstream/health_discovery_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ class HdsCluster : public Cluster, Logger::Loggable<Logger::Id::upstream> {
Ssl::ContextManager& ssl_context_manager_;
bool added_via_api_;
bool initialized_ = false;

This comment has been minimized.

Sign in to view

Copy link
@xdzhai

xdzhai Sep 7, 2020 •
edited
Loading

Contributor

From my understanding, we should be able to guarantee Initialize() is only called once after constructor.
Not sure if I missed anything, seems the situation is not changed here.
uint64_t config_hash_;
uint64_t endpoints_hash_;
uint64_t health_checkers_hash_;

HostVectorSharedPtr hosts_;
HostsPerLocalitySharedPtr hosts_per_locality_;
Expand Down

0 comments on commit 20f069f

Please sign in to comment.