Ensure Redis Proxy can follow redirection when doing request mirroring #8606

HenryYYang · 2019-10-14T13:38:03Z

Description: Currently the Redis redirection logic is part of the command splitter, this has the consequence that shadow traffic to other clusters are not able to follow the redirection responses. Part of the challenge of the change is that the original request is the storage of the original request, as it could be destroyed before the redirection command is received.

The solution is refactoring the redirection logic into the Redis ConnPool. This greatly simplify the redirection logic. Also the connection pool is the only entity that can meaningfully manage the connections to upstream and correctly drain the connections. This also require storing a shared_ptr to the incoming request with the connection pool.
Risk Level: High
Testing: Add unit tests
Docs Changes: Updated
Release Notes: Updated

Add CompositeArray to allow the RedisConnPool to maintain a shared_ptr to the request and to avoid copying the request. Added performance test Signed-off-by: Henry Yang <hyang@lyft.com>

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

jmarantz · 2019-10-14T16:56:15Z

I took the liberty of adding some markdown quotes to your PR description so the table is legible.

But below the table can you summarize your conclusions from it?

jmarantz · 2019-10-14T16:56:43Z

master merge needed.

jmarantz · 2019-10-14T16:58:19Z

drive by comment from description: does it make sense to flag-control this performance trade-off, as you indicated there are cases where the new approach is slower. Maybe that describes someone's use-case for which this is a regression?

HenryYYang · 2019-10-14T20:10:53Z

I think most of the cost is from creating the new shared_ptr, I think I can work around this and improve the performance. I'll report back.

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

# Conflicts: # source/extensions/clusters/redis/redis_cluster_lb.cc # source/extensions/filters/network/redis_proxy/conn_pool_impl.cc Signed-off-by: Henry Yang <hyang@lyft.com>

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

HenryYYang · 2019-10-17T12:26:52Z

Ok, I've update the code and the microbenchmark result. It's looking much better.

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

# Conflicts: # source/extensions/filters/network/redis_proxy/command_splitter_impl.cc # source/extensions/filters/network/redis_proxy/conn_pool.h # source/extensions/filters/network/redis_proxy/conn_pool_impl.h # test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc # test/extensions/filters/network/redis_proxy/mocks.h Signed-off-by: Henry Yang <hyang@lyft.com>

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

mattklein123

Henry, can you split out the composite array change into an independent PR? I think this PR is convolving multiple changes and it's a bit big. That would be easier to review and comment on. Thank you!

/wait

stale · 2019-10-31T20:37:48Z

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!

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

mattklein123

At a high level LGTM, just one readability comment. I would strongly recommend getting either @msukalski or @FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg to do an in depth review.

/wait

source/extensions/filters/network/common/redis/utility.cc

HenryYYang · 2019-11-26T23:56:18Z

@msukalski @FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg PTAL

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

source/extensions/filters/network/common/redis/client_impl.cc

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

test/extensions/filters/network/common/redis/mocks.cc

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

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg

I think this is an excellent simplification and can merge as is.

That said, have you had a chance to run it in your test environment and run some generated traffic against? Would be good to have confirmation on both simple and fragmented requests that it works as expected.

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

mattklein123

LGTM with small nit, thanks!

/wait

mattklein123 · 2019-12-03T00:05:43Z

test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc

+
+    EXPECT_CALL(active_request, cancel());
+    request->cancel();
+    //    Common::Redis::RespValuePtr resp_value = std::make_unique<Common::Redis::RespValue>();


del commented out code, here and elsewhere.

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

mattklein123

Thanks!

antoniovicente · 2020-01-03T00:22:37Z

test/extensions/filters/network/redis_proxy/command_split_speed_test.cc

+  void createShared(Common::Redis::RespValueSharedPtr request) {
+    for (uint64_t i = 1; i < request->asArray().size(); i += 2) {
+      auto single_set = std::make_shared<const Common::Redis::RespValue>(
+          request, Common::Redis::Utility::SetRequest::instance(), i, i + 2);


"i + 2" looks like a typo. See #9541

HenryYYang added 2 commits October 14, 2019 06:13

Refactor the redirection logic into the RedisConnPool.

293d852

Add CompositeArray to allow the RedisConnPool to maintain a shared_ptr to the request and to avoid copying the request. Added performance test Signed-off-by: Henry Yang <hyang@lyft.com>

Fix format

36ebc52

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

HenryYYang requested a review from mattklein123 as a code owner October 14, 2019 13:38

jmarantz assigned danielhochman Oct 14, 2019

HenryYYang added 8 commits October 14, 2019 14:21

Fix tests

882e42b

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

Merge branch 'master' into redirect_mirror

031ff92

# Conflicts: # source/extensions/clusters/redis/redis_cluster_lb.cc # source/extensions/filters/network/redis_proxy/conn_pool_impl.cc Signed-off-by: Henry Yang <hyang@lyft.com>

Fix move constructor

d151ae2

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

Fix clang errors

7536789

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

Fix clang errors

a9cfd91

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

Format code

5793b6b

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

Use absl::variant instead of shared_ptr to improve performance.

fac3ffb

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

Fix format

b940d04

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

HenryYYang added 6 commits October 17, 2019 16:32

Fix clang

e63f38e

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

Fix clang

dd78fb0

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

Kick CI

a1b0741

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

fix merge errors

cf4c3f6

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

Fix format

deab4b6

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

mattklein123 assigned mattklein123 and unassigned danielhochman Oct 23, 2019

Fix tests

7a80d45

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

mattklein123 requested changes Oct 24, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 24, 2019

HenryYYang mentioned this pull request Oct 27, 2019

Avoid string copy for Redis command splitter #8780

Merged

Fix failing test

47fd4d9

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

HenryYYang changed the title ~~[WIP] Ensure Redis Proxy can follow redirection when doing request mirroring~~ Ensure Redis Proxy can follow redirection when doing request mirroring Nov 13, 2019

HenryYYang added 3 commits November 14, 2019 11:19

Add failure test for ConnPoolImpl

b6f57cc

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

Merge branch 'master' into redirect_mirror

089f74b

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

sort version_history file

98ab0bc

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

mattklein123 added the no stalebot Disables stalebot from closing an issue label Nov 17, 2019

mattklein123 requested changes Nov 25, 2019

View reviewed changes

source/extensions/filters/network/common/redis/utility.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 25, 2019

Change the onDirection method signature.

93311e5

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

repokitteh-read-only bot removed the waiting label Nov 27, 2019

HenryYYang added 2 commits November 27, 2019 00:56

Merge branch 'master' into redirect_mirror

2e301c3

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

fix unit test

4896554

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

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg reviewed Nov 27, 2019

View reviewed changes

source/extensions/filters/network/common/redis/client_impl.cc Outdated Show resolved Hide resolved

fix PR feedback

d8384cb

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

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg reviewed Nov 28, 2019

View reviewed changes

test/extensions/filters/network/common/redis/mocks.cc Outdated Show resolved Hide resolved

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg reviewed Nov 28, 2019

View reviewed changes

source/extensions/filters/network/redis_proxy/command_splitter_impl.cc Show resolved Hide resolved

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg previously approved these changes Nov 28, 2019

View reviewed changes

mattklein123 added the waiting label Dec 2, 2019

fix PR feedback

2df5066

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

HenryYYang dismissed FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg’s stale review via 2df5066 December 2, 2019 23:47

repokitteh-read-only bot removed the waiting label Dec 2, 2019

mattklein123 requested changes Dec 3, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 3, 2019

fix PR feedback

eb2e2e3

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

repokitteh-read-only bot removed the waiting label Dec 6, 2019

mattklein123 approved these changes Dec 6, 2019

View reviewed changes

mattklein123 merged commit 6459645 into envoyproxy:master Dec 6, 2019

antoniovicente reviewed Jan 3, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure Redis Proxy can follow redirection when doing request mirroring #8606

Ensure Redis Proxy can follow redirection when doing request mirroring #8606

HenryYYang commented Oct 14, 2019 •

edited

Loading

jmarantz commented Oct 14, 2019

jmarantz commented Oct 14, 2019

jmarantz commented Oct 14, 2019

HenryYYang commented Oct 14, 2019

HenryYYang commented Oct 17, 2019

mattklein123 left a comment

stale bot commented Oct 31, 2019

mattklein123 left a comment

HenryYYang commented Nov 26, 2019

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg left a comment

mattklein123 left a comment

mattklein123 Dec 3, 2019

mattklein123 left a comment

antoniovicente Jan 3, 2020

Ensure Redis Proxy can follow redirection when doing request mirroring #8606

Ensure Redis Proxy can follow redirection when doing request mirroring #8606

Conversation

HenryYYang commented Oct 14, 2019 • edited Loading

jmarantz commented Oct 14, 2019

jmarantz commented Oct 14, 2019

jmarantz commented Oct 14, 2019

HenryYYang commented Oct 14, 2019

HenryYYang commented Oct 17, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

stale bot commented Oct 31, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

HenryYYang commented Nov 26, 2019

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Dec 3, 2019

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

antoniovicente Jan 3, 2020

Choose a reason for hiding this comment

HenryYYang commented Oct 14, 2019 •

edited

Loading