Redis cluster read replica by HenryYYang · Pull Request #7496 · envoyproxy/envoy

HenryYYang · 2019-07-08T22:19:23Z

Description: Add read policy to redis proxy config. Here's the definition of ReadPolicy and descriptions for each option.

// All ReadPolicy settings except MASTER may return stale data because replicas replication is
// asynchronous and requires some delay. You need to ensure that your application can tolerate
// stale data.
enum ReadPolicy {
  // Default mode. Read from the current master node.
  MASTER = 0;
  // Read from the master, but if it is unavailable, read from replica nodes.
  PREFER_MASTER = 1;
  // Read from replica nodes.
  REPLICA = 2;
  // Read from the replica nodes, but if none is unavailable, read from the master.
  PREFER_REPLICA = 3;
  // Read from any node of the cluster.
  ANY = 4;
}

Risk Level: Medium, the default will be MASTER which is same as existing case.
Testing: Added unit tests, more integration test to follow.
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Henry Yang <hyang@lyft.com>

mattklein123 · 2019-07-09T03:44:45Z

@msukalski for first pass.

/wait

Signed-off-by: Henry Yang <hyang@lyft.com>

msukalski · 2019-07-13T05:06:57Z

I'll take a look at this this weekend.

msukalski

Overall, it looks pretty good at this stage.

msukalski · 2019-07-15T02:47:31Z

api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto

    // defaults to 3ms.
    google.protobuf.Duration buffer_flush_timeout = 5 [(gogoproto.stdduration) = true];
+
+    // All ReadPolicy settings except MASTER may return stale data because replicas replication is


nit: "replicas" appears redundant.

source/extensions/clusters/redis/redis_cluster.h

msukalski · 2019-07-15T23:15:22Z

source/extensions/clusters/redis/redis_cluster_lb.cc

-    current = slot_array_;
+  // The slots is sorted, allowing for a quick comparison to make sure we need to update the slot
+  // array sort based on start and end to enable efficient comparison
+  sort(slots->begin(), slots->end(), [](const ClusterSlot& lhs, const ClusterSlot& rhs) -> bool {


It's not particularly clear whether this sort() is worth it. Mostly I don't see any ordering dependencies in this code that the sort() might alleviate. Your thoughts, please?

The sort() is useful for the current_cluster_slot_ && *current_cluster_slot_ == *slots.

Let's say K(16384) slots are broken down to M ranges which are assigned to N shards(1 master + 0-n replicas). M and N should normally be < 100 except for the largest clusters.

If we sort the M ranges based on the start and end for each range, then the comparison of whether allocation is changed is O(M log M) + O(M) without allocating additional space. I can't be certain that "Cluster Slots" return a sorted range, as that's not part of the contract. If it does return a sorted range, the sort function is going to be O(M) in most implementations, then it's a small cost to pay.

msukalski · 2019-07-15T23:19:09Z

source/extensions/clusters/redis/redis_cluster_lb.cc

+      if (!shard->replicas_.healthyHosts().empty()) {
+        return chooseRandomHost(shard->replicas_, random_);
+      } else {
+        return chooseRandomHost(shard->all_hosts_, random_);


Is there a reason for the lack of symmetry with respect to PreferMaster? In other words, why does the no replicas available in this scenario not return shard->master_ instead of chooseRandomHost(shard->all_hosts_, random_)?

by doing chooseRandomHost(shard->all_hosts_, random_) will make sure that we pick a random host if the master and replicas are all unhealthy. If only the master is healthy, then that's the host that will be picked.

msukalski · 2019-07-15T23:30:37Z

source/extensions/filters/network/redis_proxy/conn_pool_impl.cc

    }
+    // Any connection to replica requires the READONLY command in order to perform read.
+    // Also the READONLY command is a no-opt for the master.
+    // We only need to send the READONLY command iff it's possible that the host is a replica.


I think this is a valid conclusion after looking at the redis 4 server code.

Thanks for verifying

stale · 2019-07-29T21:57:04Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

…k status change. Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only · 2019-07-31T18:44:09Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only · 2019-07-31T20:59:23Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

htuch · 2019-07-31T22:04:02Z

@envoyproxy/api-shepherds API LGTM

repokitteh-read-only · 2019-08-13T18:54:57Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

repokitteh-read-only · 2019-08-13T18:54:57Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

repokitteh-read-only · 2019-08-14T18:15:33Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

Signed-off-by: Henry Yang <hyang@lyft.com>

Co-Authored-By: Matt Klein <mattklein123@gmail.com> Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only · 2019-08-14T18:57:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only · 2019-08-14T19:58:25Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only · 2019-08-15T02:03:54Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

mattklein123 · 2019-08-15T19:26:15Z

Please merge master.

/wait

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only · 2019-08-15T21:45:57Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7496 was synchronize by HenryYYang.

see: more, trace.

mattklein123

Nice, thanks!

HenryYYang added 2 commits July 3, 2019 11:55

add redis-cluster-read-replica

86d4706

Signed-off-by: Henry Yang <hyang@lyft.com>

Merge branch 'master' into redis-cluster-read-replica

773a32b

Signed-off-by: Henry Yang <hyang@lyft.com>

HenryYYang requested a review from mattklein123 as a code owner July 8, 2019 22:19

mattklein123 assigned mattklein123 and msukalski Jul 9, 2019

repokitteh-read-only bot added the waiting label Jul 9, 2019

Fix tests

8b1701f

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only bot removed the waiting label Jul 9, 2019

Fix tests.

88a9c55

Signed-off-by: Henry Yang <hyang@lyft.com>

mattklein123 added the waiting label Jul 10, 2019

Fix ipv6 address issue with macos

ccdcdb1

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only bot removed the waiting label Jul 11, 2019

mattklein123 added the waiting label Jul 11, 2019

msukalski reviewed Jul 15, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 29, 2019

Add more redis cluster lb tests and reload the shard when health chec…

4590c51

…k status change. Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only bot removed the waiting label Jul 31, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 31, 2019

HenryYYang changed the title ~~WIP: Redis cluster read replica~~ Redis cluster read replica Jul 31, 2019

mattklein123 added the waiting label Jul 31, 2019

Merge branch 'master' into redis-cluster-read-replica

4f9bf1f

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only bot removed the waiting label Jul 31, 2019

mattklein123 added the waiting label Jul 31, 2019

repokitteh-read-only bot removed the waiting label Aug 1, 2019

mattklein123 added the waiting label Aug 14, 2019

repokitteh-read-only bot removed the waiting label Aug 14, 2019

mattklein123 added the waiting label Aug 14, 2019

HenryYYang and others added 6 commits August 14, 2019 11:55

Fix clang and format, improve test coverage.

f4368fe

Signed-off-by: Henry Yang <hyang@lyft.com>

Fix PR feedbacks.

f862d9e

Signed-off-by: Henry Yang <hyang@lyft.com>

Fix PR feedback, added tests

b53fa6f

Signed-off-by: Henry Yang <hyang@lyft.com>

Update api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto

5619ec5

Co-Authored-By: Matt Klein <mattklein123@gmail.com> Signed-off-by: Henry Yang <hyang@lyft.com>

Update docs/root/intro/arch_overview/other_protocols/redis.rst

29b176a

Co-Authored-By: Matt Klein <mattklein123@gmail.com> Signed-off-by: Henry Yang <hyang@lyft.com>

Update docs/root/intro/version_history.rst

5909e01

Co-Authored-By: Matt Klein <mattklein123@gmail.com> Signed-off-by: Henry Yang <hyang@lyft.com>

HenryYYang force-pushed the redis-cluster-read-replica branch from 5ea702e to 5909e01 Compare August 14, 2019 18:57

repokitteh-read-only bot removed the waiting label Aug 14, 2019

repokitteh-read-only bot added the waiting label Aug 14, 2019

Merge branch 'master' into redis-cluster-read-replica

b3c0bac

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only bot removed the waiting label Aug 14, 2019

Fix review feedback

1baa468

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only bot added the waiting label Aug 15, 2019

Merge branch 'master' into redis-cluster-read-replica

66560f9

Signed-off-by: Henry Yang <hyang@lyft.com>

repokitteh-read-only bot removed the waiting label Aug 15, 2019

mattklein123 approved these changes Aug 15, 2019

View reviewed changes

mattklein123 merged commit 560d09f into envoyproxy:master Aug 15, 2019

lizan mentioned this pull request Aug 15, 2019

redis: fix merge race #7943

Merged

wbertelsen mentioned this pull request Oct 9, 2019

Linker error building on system lacking a c++11 toolchain #8558

Closed

Conversation

HenryYYang commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Jul 9, 2019

Uh oh!

msukalski commented Jul 13, 2019

Uh oh!

msukalski left a comment

Choose a reason for hiding this comment

Uh oh!

msukalski Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msukalski Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

HenryYYang Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

msukalski Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

HenryYYang Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

msukalski Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

HenryYYang Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 31, 2019

Uh oh!

repokitteh-read-only bot commented Jul 31, 2019

Uh oh!

htuch commented Jul 31, 2019

Uh oh!

repokitteh-read-only bot commented Aug 13, 2019

Uh oh!

repokitteh-read-only bot commented Aug 13, 2019

Uh oh!

repokitteh-read-only bot commented Aug 14, 2019

Uh oh!

repokitteh-read-only bot commented Aug 14, 2019

Uh oh!

repokitteh-read-only bot commented Aug 14, 2019

Uh oh!

repokitteh-read-only bot commented Aug 15, 2019

Uh oh!

mattklein123 commented Aug 15, 2019

Uh oh!

repokitteh-read-only bot commented Aug 15, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HenryYYang commented Jul 8, 2019 •

edited

Loading