From 5c326e9146e76589ebfe678ff919b2c0da771efd Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Thu, 22 Apr 2021 14:39:31 +0800 Subject: [PATCH 01/10] redis cluster: fix ClusterSlot operator == Signed-off-by: gaoliangdut --- .../extensions/clusters/redis/redis_cluster_lb.cc | 14 ++++++++++++-- .../clusters/redis/redis_cluster_lb_test.cc | 9 ++++++--- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index 43bb0f9b32224..37781ee81354e 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -6,8 +6,18 @@ namespace Clusters { namespace Redis { bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSlot& rhs) const { - return start_ == rhs.start_ && end_ == rhs.end_ && primary_ == rhs.primary_ && - replicas_ == rhs.replicas_; + if (start_ != rhs.start_ || end_ != rhs.end_ || *primary_ != *rhs.primary_ || + replicas_.size() != rhs.replicas_.size()) { + return false; + } else { + for (auto it1 = replicas_.begin(), it2 = rhs.replicas_.begin(); it1 != replicas_.end(); + it1++, it2++) { + if (**it1 != **it2) { + return false; + } + } + return true; + } } // RedisClusterLoadBalancerFactory diff --git a/test/extensions/clusters/redis/redis_cluster_lb_test.cc b/test/extensions/clusters/redis/redis_cluster_lb_test.cc index a0ed75aedf1f9..f26b70593270d 100644 --- a/test/extensions/clusters/redis/redis_cluster_lb_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_lb_test.cc @@ -349,6 +349,9 @@ TEST_F(RedisClusterLoadBalancerTest, ClusterSlotUpdate) { TEST_F(RedisClusterLoadBalancerTest, ClusterSlotNoUpdate) { Upstream::HostVector hosts{Upstream::makeTestHost(info_, "tcp://127.0.0.1:90", simTime()), + Upstream::makeTestHost(info_, "tcp://127.0.0.1:91", simTime()), + Upstream::makeTestHost(info_, "tcp://127.0.0.1:92", simTime()), + Upstream::makeTestHost(info_, "tcp://127.0.0.1:90", simTime()), Upstream::makeTestHost(info_, "tcp://127.0.0.1:91", simTime()), Upstream::makeTestHost(info_, "tcp://127.0.0.1:92", simTime())}; @@ -373,9 +376,9 @@ TEST_F(RedisClusterLoadBalancerTest, ClusterSlotNoUpdate) { // Calling cluster slot update without change should not change assignment. std::vector updated_slot{ - ClusterSlot(0, 1000, hosts[0]->address()), - ClusterSlot(1001, 2000, hosts[1]->address()), - ClusterSlot(2001, 16383, hosts[2]->address()), + ClusterSlot(0, 1000, hosts[3]->address()), + ClusterSlot(1001, 2000, hosts[4]->address()), + ClusterSlot(2001, 16383, hosts[5]->address()), }; EXPECT_EQ(false, factory_->onClusterSlotUpdate( std::make_unique>(updated_slot), all_hosts)); From c6f431fe82e068c47e5715963960533b05a5176f Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Fri, 23 Apr 2021 09:59:54 +0800 Subject: [PATCH 02/10] redis cluster: remove else Signed-off-by: gaoliangdut --- .../extensions/clusters/redis/redis_cluster_lb.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index 37781ee81354e..fd9b6a90e42c0 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -9,15 +9,14 @@ bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSl if (start_ != rhs.start_ || end_ != rhs.end_ || *primary_ != *rhs.primary_ || replicas_.size() != rhs.replicas_.size()) { return false; - } else { - for (auto it1 = replicas_.begin(), it2 = rhs.replicas_.begin(); it1 != replicas_.end(); - it1++, it2++) { - if (**it1 != **it2) { - return false; - } + } + for (auto it1 = replicas_.begin(), it2 = rhs.replicas_.begin(); it1 != replicas_.end(); + it1++, it2++) { + if (**it1 != **it2) { + return false; } - return true; } + return true; } // RedisClusterLoadBalancerFactory From 6dbef9956e7887e43c32a22860466f969d46c7a0 Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Fri, 23 Apr 2021 17:20:47 +0800 Subject: [PATCH 03/10] redis cluster: fix replicas compare Signed-off-by: gaoliangdut --- source/extensions/clusters/redis/redis_cluster_lb.cc | 12 +++++++++--- .../clusters/redis/redis_cluster_lb_test.cc | 11 +++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index fd9b6a90e42c0..fd3ee93268ef9 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -10,12 +10,18 @@ bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSl replicas_.size() != rhs.replicas_.size()) { return false; } - for (auto it1 = replicas_.begin(), it2 = rhs.replicas_.begin(); it1 != replicas_.end(); - it1++, it2++) { - if (**it1 != **it2) { + + std::set replicas_set; + for (auto const& replica : replicas_) { + replicas_set.emplace(replica->asString()); + } + + for (auto const& replica : rhs.replicas_) { + if (replicas_set.find(replica->asString()) == replicas_set.end()) { return false; } } + return true; } diff --git a/test/extensions/clusters/redis/redis_cluster_lb_test.cc b/test/extensions/clusters/redis/redis_cluster_lb_test.cc index f26b70593270d..1b7abc6bb9420 100644 --- a/test/extensions/clusters/redis/redis_cluster_lb_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_lb_test.cc @@ -354,16 +354,25 @@ TEST_F(RedisClusterLoadBalancerTest, ClusterSlotNoUpdate) { Upstream::makeTestHost(info_, "tcp://127.0.0.1:90", simTime()), Upstream::makeTestHost(info_, "tcp://127.0.0.1:91", simTime()), Upstream::makeTestHost(info_, "tcp://127.0.0.1:92", simTime())}; + Upstream::HostVector replicas{Upstream::makeTestHost(info_, "tcp://127.0.0.2:90", simTime()), + Upstream::makeTestHost(info_, "tcp://127.0.0.2:91", simTime()), + Upstream::makeTestHost(info_, "tcp://127.0.0.2:90", simTime()), + Upstream::makeTestHost(info_, "tcp://127.0.0.2:91", simTime())}; ClusterSlotsPtr slots = std::make_unique>(std::vector{ ClusterSlot(0, 1000, hosts[0]->address()), ClusterSlot(1001, 2000, hosts[1]->address()), ClusterSlot(2001, 16383, hosts[2]->address()), }); + + (*slots)[0].addReplica(replicas[0]->address()); + (*slots)[0].addReplica(replicas[1]->address()); Upstream::HostMap all_hosts{ {hosts[0]->address()->asString(), hosts[0]}, {hosts[1]->address()->asString(), hosts[1]}, {hosts[2]->address()->asString(), hosts[2]}, + {replicas[0]->address()->asString(), replicas[0]}, + {replicas[1]->address()->asString(), replicas[1]}, }; // A list of (hash: host_index) pair. @@ -380,6 +389,8 @@ TEST_F(RedisClusterLoadBalancerTest, ClusterSlotNoUpdate) { ClusterSlot(1001, 2000, hosts[4]->address()), ClusterSlot(2001, 16383, hosts[5]->address()), }; + updated_slot[0].addReplica(replicas[3]->address()); + updated_slot[0].addReplica(replicas[2]->address()); EXPECT_EQ(false, factory_->onClusterSlotUpdate( std::make_unique>(updated_slot), all_hosts)); validateAssignment(hosts, expected_assignments); From bb75fe2fb43c06cb80431effe895f3a5e9fd7393 Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Sun, 25 Apr 2021 11:37:03 +0800 Subject: [PATCH 04/10] redis cluster: use equality function Signed-off-by: gaoliangdut --- .../extensions/clusters/redis/redis_cluster_lb.cc | 7 +------ .../extensions/clusters/redis/redis_cluster_lb.h | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index fd3ee93268ef9..7d6071a50c81d 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -11,13 +11,8 @@ bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSl return false; } - std::set replicas_set; - for (auto const& replica : replicas_) { - replicas_set.emplace(replica->asString()); - } - for (auto const& replica : rhs.replicas_) { - if (replicas_set.find(replica->asString()) == replicas_set.end()) { + if (replicas_.find(replica) == replicas_.end()) { return false; } } diff --git a/source/extensions/clusters/redis/redis_cluster_lb.h b/source/extensions/clusters/redis/redis_cluster_lb.h index 561de3b681e53..d823b85ed23d5 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.h +++ b/source/extensions/clusters/redis/redis_cluster_lb.h @@ -37,7 +37,18 @@ class ClusterSlot { int64_t start() const { return start_; } int64_t end() const { return end_; } Network::Address::InstanceConstSharedPtr primary() const { return primary_; } - const absl::flat_hash_set& replicas() const { + struct Hash { + size_t operator()(const Network::Address::InstanceConstSharedPtr& address) const { + return absl::Hash()(address->asString()); + } + }; + struct Eq { + bool operator()(const Network::Address::InstanceConstSharedPtr& lhs, + const Network::Address::InstanceConstSharedPtr& rhs) const { + return *lhs == *rhs; + } + }; + const absl::flat_hash_set& replicas() const { return replicas_; } void addReplica(Network::Address::InstanceConstSharedPtr replica_address) { @@ -50,7 +61,7 @@ class ClusterSlot { int64_t start_; int64_t end_; Network::Address::InstanceConstSharedPtr primary_; - absl::flat_hash_set replicas_; + absl::flat_hash_set replicas_; }; using ClusterSlotsPtr = std::unique_ptr>; From 4c198900cea205480887c9f06bedae290e82064c Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Wed, 28 Apr 2021 16:59:44 +0800 Subject: [PATCH 05/10] redis cluster: change flat_hash_set to map Signed-off-by: gaoliangdut --- .../extensions/clusters/redis/redis_cluster.cc | 4 ++-- .../clusters/redis/redis_cluster_lb.cc | 4 ++-- .../clusters/redis/redis_cluster_lb.h | 17 +++-------------- 3 files changed, 7 insertions(+), 18 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 297248a6b6a15..822eab1166be6 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -100,8 +100,8 @@ void RedisCluster::onClusterSlotUpdate(ClusterSlotsPtr&& slots) { new_hosts.emplace_back(new RedisHost(info(), "", slot.primary(), *this, true, time_source_)); all_new_hosts.emplace(slot.primary()->asString()); for (auto const& replica : slot.replicas()) { - new_hosts.emplace_back(new RedisHost(info(), "", replica, *this, false, time_source_)); - all_new_hosts.emplace(replica->asString()); + new_hosts.emplace_back(new RedisHost(info(), "", replica.second, *this, false, time_source_)); + all_new_hosts.emplace(replica.first); } } diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index 7d6071a50c81d..83385d65ba09b 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -12,7 +12,7 @@ bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSl } for (auto const& replica : rhs.replicas_) { - if (replicas_.find(replica) == replicas_.end()) { + if (replicas_.find(replica.first) == replicas_.end()) { return false; } } @@ -53,7 +53,7 @@ bool RedisClusterLoadBalancerFactory::onClusterSlotUpdate(ClusterSlotsPtr&& slot primary_and_replicas->push_back(primary_host->second); for (auto const& replica : slot.replicas()) { - auto replica_host = all_hosts.find(replica->asString()); + auto replica_host = all_hosts.find(replica.first); ASSERT(replica_host != all_hosts.end(), "we expect all address to be found in the updated_hosts"); replicas->push_back(replica_host->second); diff --git a/source/extensions/clusters/redis/redis_cluster_lb.h b/source/extensions/clusters/redis/redis_cluster_lb.h index d823b85ed23d5..64c8d3fe26b0b 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.h +++ b/source/extensions/clusters/redis/redis_cluster_lb.h @@ -37,22 +37,11 @@ class ClusterSlot { int64_t start() const { return start_; } int64_t end() const { return end_; } Network::Address::InstanceConstSharedPtr primary() const { return primary_; } - struct Hash { - size_t operator()(const Network::Address::InstanceConstSharedPtr& address) const { - return absl::Hash()(address->asString()); - } - }; - struct Eq { - bool operator()(const Network::Address::InstanceConstSharedPtr& lhs, - const Network::Address::InstanceConstSharedPtr& rhs) const { - return *lhs == *rhs; - } - }; - const absl::flat_hash_set& replicas() const { + const std::map& replicas() const { return replicas_; } void addReplica(Network::Address::InstanceConstSharedPtr replica_address) { - replicas_.insert(std::move(replica_address)); + replicas_.emplace(replica_address->asString(), std::move(replica_address)); } bool operator==(const ClusterSlot& rhs) const; @@ -61,7 +50,7 @@ class ClusterSlot { int64_t start_; int64_t end_; Network::Address::InstanceConstSharedPtr primary_; - absl::flat_hash_set replicas_; + std::map replicas_; }; using ClusterSlotsPtr = std::unique_ptr>; From 6594417045c3e865e73790e965820355b5b7cc2d Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Thu, 29 Apr 2021 18:46:22 +0800 Subject: [PATCH 06/10] redis cluster: compare with std::equal Signed-off-by: gaoliangdut --- source/extensions/clusters/redis/redis_cluster_lb.cc | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index 83385d65ba09b..61884b2964f9d 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -11,13 +11,8 @@ bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSl return false; } - for (auto const& replica : rhs.replicas_) { - if (replicas_.find(replica.first) == replicas_.end()) { - return false; - } - } - - return true; + return std::equal(replicas_.begin(), replicas_.end(), rhs.replicas_.begin(), rhs.replicas_.end(), + [](const auto& it1, const auto& it2) { return it1.first == it2.first; }); } // RedisClusterLoadBalancerFactory From 732d8e74c22d65897d6dcfcb3a7811917a5b7fb5 Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Mon, 10 May 2021 14:29:59 +0800 Subject: [PATCH 07/10] change std::map to absl::btree_map Signed-off-by: gaoliangdut --- source/extensions/clusters/redis/redis_cluster_lb.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/extensions/clusters/redis/redis_cluster_lb.h b/source/extensions/clusters/redis/redis_cluster_lb.h index 64c8d3fe26b0b..7ca27407d1612 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.h +++ b/source/extensions/clusters/redis/redis_cluster_lb.h @@ -18,8 +18,7 @@ #include "extensions/filters/network/common/redis/codec.h" #include "extensions/filters/network/common/redis/supported_commands.h" -#include "absl/container/flat_hash_map.h" -#include "absl/container/flat_hash_set.h" +#include "absl/container/btree_map.h" #include "absl/synchronization/mutex.h" namespace Envoy { @@ -37,7 +36,7 @@ class ClusterSlot { int64_t start() const { return start_; } int64_t end() const { return end_; } Network::Address::InstanceConstSharedPtr primary() const { return primary_; } - const std::map& replicas() const { + const absl::btree_map& replicas() const { return replicas_; } void addReplica(Network::Address::InstanceConstSharedPtr replica_address) { @@ -50,7 +49,7 @@ class ClusterSlot { int64_t start_; int64_t end_; Network::Address::InstanceConstSharedPtr primary_; - std::map replicas_; + absl::btree_map replicas_; }; using ClusterSlotsPtr = std::unique_ptr>; From 46e9ff633ae54fb87f34fc213ce75ad54b63cc85 Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Tue, 11 May 2021 16:00:28 +0800 Subject: [PATCH 08/10] Kick CI Signed-off-by: gaoliangdut From ba8b1620c45918d284d463d8f44f01052ec8b1c7 Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Thu, 13 May 2021 09:54:10 +0800 Subject: [PATCH 09/10] add a comment for ClusterSlot compare Signed-off-by: gaoliangdut --- source/extensions/clusters/redis/redis_cluster_lb.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index 61884b2964f9d..25f38f448dae0 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -10,7 +10,8 @@ bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSl replicas_.size() != rhs.replicas_.size()) { return false; } - + // The value type is shared_ptr, and the shared_ptr is not same one even for same ip:port. + // so, just compare the key here. return std::equal(replicas_.begin(), replicas_.end(), rhs.replicas_.begin(), rhs.replicas_.end(), [](const auto& it1, const auto& it2) { return it1.first == it2.first; }); } From fa5516325761cfa30aeba91bf83b701ff7881b7c Mon Sep 17 00:00:00 2001 From: gaoliangdut Date: Sun, 16 May 2021 14:31:37 +0800 Subject: [PATCH 10/10] Kick CI Signed-off-by: gaoliangdut