redis cluster: fix ClusterSlot operator == by gaoliangdut · Pull Request #16116 · envoyproxy/envoy

gaoliangdut · 2021-04-22T07:17:05Z

Signed-off-by: gaoliangdut gaoliang_dlut@126.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:The 'primary_' and element in 'replicas_' is type of shared_ptr, this shared_ptr is created from different redis response，so it will always returns false.
Additional Description:
Risk Level: Low
Testing: unit test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

repokitteh-read-only · 2021-04-22T07:17:08Z

Hi @gaoliangdut, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16116 was opened by gaoliangdut.

see: more, trace.

asraa · 2021-04-22T13:52:28Z

/assign @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2021-04-22T13:52:30Z

@envoyproxy/first-pass-reviewers cannot be assigned to this issue.

🐱

Caused by: a #16116 (comment) was created by @asraa.

see: more, trace.

rojkov

Thank you! Just one suggestion added.

rojkov · 2021-04-22T14:12:33Z

source/extensions/clusters/redis/redis_cluster_lb.cc

+  } else {
+    for (auto it1 = replicas_.begin(), it2 = rhs.replicas_.begin(); it1 != replicas_.end();
+         it1++, it2++) {
+      if (**it1 != **it2) {
+        return false;
+      }
+    }


nit: this loop shouldn't be nested in else to lessen cognitive load.

Thank you for your suggestion, I have add a new commit.

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov · 2021-04-23T07:44:33Z

source/extensions/clusters/redis/redis_cluster_lb.cc

+  for (auto it1 = replicas_.begin(), it2 = rhs.replicas_.begin(); it1 != replicas_.end();
+       it1++, it2++) {
+    if (**it1 != **it2) {


replicas_ is a absl::flat_hash_set which doesn't make any guarantees about iteration order. How can we be sure it's going to work always?

Oh, that's a problem. how about use this code instead?
`
std::setstd::string 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;
`

I have add a new commit.

Probably we could keep the existing code and to instantiate replicas_ as a absl::flat_hash_set with a custom equality function instead?

OK, I have understood what you said, I will try it in this way.

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

asraa · 2021-04-24T22:50:18Z

Assigning @snowp for second/final pass

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov · 2021-04-26T12:23:31Z

source/extensions/clusters/redis/redis_cluster_lb.cc

+  for (auto const& replica : rhs.replicas_) {
+    if (replicas_.find(replica) == replicas_.end()) {
+      return false;
+    }
+  }


Isn't this loop equivalent to just

return replicas_ == rhs.replicas_;

In the test case, "replicas_ == rhs.replicas_" returns false, but In my code, it returns true. I'm trying to find the reason for this.

This is really not equal. Because in absl, "operator ==" uses "==" to compare the value, and "find" uses "eq_ref" to compare the value.

jmarantz · 2021-04-26T12:27:39Z

source/extensions/clusters/redis/redis_cluster_lb.h

-  const absl::flat_hash_set<Network::Address::InstanceConstSharedPtr>& replicas() const {
+  struct Hash {
+    size_t operator()(const Network::Address::InstanceConstSharedPtr& address) const {
+      return absl::Hash<std::string>()(address->asString());


As a matter of good practice, you'd want to use return absl::Hash<absl::string_view>()(address->asStringView()); to avoid potentially creating a string temp while hashing. Though actually the name is held in a string in the main impl, so the asString() impl currently won't do that.

But in researching this I found a potential latent crash here. The doc for asString() says:

* @return a human readable string for the address that represents the * physical/resolved address. (This will not necessarily include port * information, if applicable, since that may not be resolved until bind()).

This suggests to me that bind() might mutate the name. If that happens after we have already stored an address in a hash-table, I think this can crash in subsequent hash-table operations.

So my suggestion is to convert this to a map<string,Network::Address::InstanceConstSharedPtr>. This will result in string-copies for the map key, but will avoid crashing Envoy if bind() mutates the name. And if you do that you won't need this custom hasher/comparator.

+1

That's a good catch! There's no good in hashing keys which can mutate.

Thanks for this suggestion, I have use map instead of flat_hash_set and create a commit.

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov · 2021-04-29T07:17:01Z

source/extensions/clusters/redis/redis_cluster_lb.cc

+  for (auto const& replica : rhs.replicas_) {
+    if (replicas_.find(replica.first) == replicas_.end()) {
+      return false;
+    }
+  }


suggestion: Currently this is O(n*log n). If the replicas are std::map then we can rely on how they are ordered like in your first commit to make this loop O(n).

It's a good suggestion, thank you! I have fixed it and created a new commit.

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov

Thank you! I think it's ready for a second pass @snowp

snowp

A few comments questions but overall this looks right!

snowp · 2021-04-30T12:49:34Z

source/extensions/clusters/redis/redis_cluster_lb.cc

+  return std::equal(replicas_.begin(), replicas_.end(), rhs.replicas_.begin(), rhs.replicas_.end(),
+                    [](const auto& it1, const auto& it2) { return it1.first == it2.first; });


This might have been covered in PR discussions already but how does this differ from just doing replicas_ = rhs.replicas?

The value type is shared_ptr, and the shared_ptr is not name one even for same ip:port.

Oh I see this is just comparing the key. Perhaps add a comment to make this clearer?

Ping on this ^^

Would be great to have this covered by a comment

Sorry, I forgot that, now I have add a comment. Thank you!

snowp · 2021-04-30T12:51:56Z

source/extensions/clusters/redis/redis_cluster_lb.h

  int64_t end_;
  Network::Address::InstanceConstSharedPtr primary_;
-  absl::flat_hash_set<Network::Address::InstanceConstSharedPtr> replicas_;
+  std::map<std::string, Network::Address::InstanceConstSharedPtr> replicas_;


I think normally we perfer to use the absl maps, one of absl::flat_hash_map, absl::node_hash_map if you want pointer stability or absl::btree_map if you want an ordered map.

I have changed it to absl::btree_map and created a new commit.

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

snowp

Thanks for explaining, just one question. Can you also take a look at CI to see if its a real issue?

gaoliangdut · 2021-05-11T03:18:59Z

Thanks for explaining, just one question. Can you also take a look at CI to see if its a real issue?

I have checked it, and I think so.

rojkov · 2021-05-11T06:54:56Z

/retest

repokitteh-read-only · 2021-05-11T06:55:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16116 (comment) was created by @rojkov.

see: more, trace.

rojkov · 2021-05-11T07:20:31Z

@gaoliangdut Looks like CI has ignored my retest comment. Could you please push an empty comment to trigger the tests?

git commit -s --allow-empty -m 'Kick CI' && git push

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

gaoliangdut · 2021-05-11T08:08:57Z

@gaoliangdut Looks like CI has ignored my retest comment. Could you please push an empty comment to trigger the tests?
git commit -s --allow-empty -m 'Kick CI' && git push

done

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

snowp

Thanks! Can you merge main? I think that should fix the gcc issue in CI

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

The 'primary_' and element in 'replicas_' were `shared_ptr`s, this shared_ptr is created from different redis responses, so it would always returns false. Signed-off-by: gaoliangdut <gaoliang_dlut@126.com> Signed-off-by: Sixiang Gu <sgu@twitter.com>

The 'primary_' and element in 'replicas_' were `shared_ptr`s, this shared_ptr is created from different redis responses, so it would always returns false. Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

redis cluster: fix ClusterSlot operator ==

5c326e9

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

gaoliangdut requested a review from mattklein123 as a code owner April 22, 2021 07:17

rojkov reviewed Apr 22, 2021

View reviewed changes

asraa assigned rojkov Apr 22, 2021

redis cluster: remove else

c6f431f

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov reviewed Apr 23, 2021

View reviewed changes

redis cluster: fix replicas compare

6dbef99

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

asraa assigned snowp Apr 24, 2021

redis cluster: use equality function

bb75fe2

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov reviewed Apr 26, 2021

View reviewed changes

jmarantz reviewed Apr 26, 2021

View reviewed changes

redis cluster: change flat_hash_set to map

4c19890

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov reviewed Apr 29, 2021

View reviewed changes

redis cluster: compare with std::equal

6594417

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

rojkov previously approved these changes Apr 29, 2021

View reviewed changes

snowp suggested changes Apr 30, 2021

View reviewed changes

snowp added the waiting label May 3, 2021

change std::map to absl::btree_map

732d8e7

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

gaoliangdut dismissed rojkov’s stale review via 732d8e7 May 10, 2021 06:30

repokitteh-read-only bot removed the waiting label May 10, 2021

snowp suggested changes May 10, 2021

View reviewed changes

Kick CI

46e9ff6

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

add a comment for ClusterSlot compare

ba8b162

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

snowp approved these changes May 14, 2021

View reviewed changes

gaoliangdut and others added 2 commits May 16, 2021 11:34

Merge branch 'envoyproxy:main' into main

61e7bc4

Kick CI

fa55163

Signed-off-by: gaoliangdut <gaoliang_dlut@126.com>

snowp merged commit c5833f2 into envoyproxy:main May 17, 2021

		return std::equal(replicas_.begin(), replicas_.end(), rhs.replicas_.begin(), rhs.replicas_.end(),
		[](const auto& it1, const auto& it2) { return it1.first == it2.first; });

Conversation

gaoliangdut commented Apr 22, 2021

Uh oh!

repokitteh-read-only bot commented Apr 22, 2021

Uh oh!

asraa commented Apr 22, 2021

Uh oh!

repokitteh-read-only bot commented Apr 22, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asraa commented Apr 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

gaoliangdut commented May 11, 2021

Uh oh!

rojkov commented May 11, 2021

Uh oh!

repokitteh-read-only bot commented May 11, 2021

Uh oh!

rojkov commented May 11, 2021

Uh oh!

gaoliangdut commented May 11, 2021

Uh oh!

snowp left a comment

snowp May 10, 2021 •

edited

Loading