Skip to content

Refactor Redis commands and connection pool#6055

Closed
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg wants to merge 6 commits intoenvoyproxy:masterfrom
HenryYYang:refactor-redis-commands-to-common
Closed

Refactor Redis commands and connection pool#6055
FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg wants to merge 6 commits intoenvoyproxy:masterfrom
HenryYYang:refactor-redis-commands-to-common

Conversation

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Copy link
Copy Markdown
Contributor

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg commented Feb 25, 2019 •
edited
Loading

This is part of the effort to build a proxy for Redis Cluster (#5697).

The new proxy will live in a folder next to the old Redis proxy, as opposed to simply being a config change or flag. The rationale for creating two separate proxies is as follows. Redis Cluster has additional commands which will be internal to this proxy, which essentially functions as a smart client. It needs to maintain the cluster topology as well as sending commands to nodes to go from readonly to readwrite mode on change from replica to master. As we develop the new proxy we prefer to keep the changes separate from the old one so as not to affect other users. We can always merge the two later on.

In this PR, we move common Redis code (connection pools, command splitting, etc.) to a common folder, and change the related dependencies in redis_proxy, healthcheck and tests. In future PRs we will reuse the common code in the new proxy.

FYI @HenryYYang @mattklein123 @danielhochman

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Copy link
Copy Markdown
Contributor Author

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg commented Feb 27, 2019

🔨

@mattklein123 mattklein123 self-assigned this Feb 27, 2019
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Feb 28, 2019

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg generally LGTM. Please merge master and I will take a look.

/wait

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Feb 28, 2019

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg apologies but you will need to fix DCO. Sorry for the trouble. It might be worth getting a few tips from @HenryYYang if you are sitting next to him.

/wait

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Copy link
Copy Markdown
Contributor Author

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg commented Feb 28, 2019

Yeah, I'm looking at it :|

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Copy link
Copy Markdown
Contributor Author

FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg commented Feb 28, 2019

Coverage has a (new) failure with a test unrelated to my changes. @mattklein123 I'll take a look at this after lunch.

test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc:505: Failure
Expected equality of these values:
  trace.socket_buffered_trace().events().size()
    Which is: 0
  2
[  FAILED  ] IpVersions/SslTapIntegrationTest.TruncationWithMultipleDataFrames/IPv4, where GetParam() = 4-byte object <00-00 00-00> (68 ms)

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Feb 28, 2019

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg merge master again, I just fixed that flake.

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Add Redis commands to common
7948f51 
wqupdate redis_proxy so that it builds, but tests will fail

Signed-off-by: Nicolas Flacco <nflacco@lyft.com>
@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Source compiles minus healthcheck, tests for common run but not healt…
9b669fd 
…hcheck or proxy

Signed-off-by: Nicolas Flacco <nflacco@lyft.com>
@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Tests for common and redis_proxy run, todo healthcheck
0f1de0d 
Signed-off-by: Nicolas Flacco <nflacco@lyft.com>
@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
Make the health check compile and test
72d5bf4 
Signed-off-by: Nicolas Flacco <nflacco@lyft.com>
@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
remove unneeded readme
e93f06c 
Signed-off-by: Nicolas Flacco <nflacco@lyft.com>
@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg
clang-format
3354f9c 
Signed-off-by: Nicolas Flacco <nflacco@lyft.com>
@HenryYYang
Copy link
Copy Markdown
Contributor

HenryYYang commented Mar 1, 2019

The implementation of InstanceImpl in conn_pool_impl.h and conn_pool_impl.cc are specific to redis proxy and it incompatible with redis cluster, we should leave them in the redis proxy filter.

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Mar 1, 2019

@HenryYYang per offline convo please review first and then I will take a final pass.

/wait

Merged
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Mar 4, 2019

I believe we are going with the other opened PR so closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@mattklein123 mattklein123 Awaiting requested review from mattklein123

Assignees

@HenryYYang HenryYYang

@mattklein123 mattklein123

Labels

waiting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg @mattklein123 @HenryYYang