redis: prefixed routing#6413
Merged
mattklein123 merged 13 commits intoenvoyproxy:masterfrom Apr 16, 2019
Merged
Conversation
added 3 commits
March 27, 2019 17:36
Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
Member
|
Yeah let's finish review of #6294 first and wait to get an integration test in before we work on this. |
added 2 commits
April 10, 2019 09:57
…uting-take-2 Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
mattklein123
requested changes
Apr 11, 2019
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks looks great. Can we please add an integration test for this feature? I don't think it should be that hard now that @msukalski add an integration test that uses multiple upstreams. Thank you!
/wait
DEPRECATED.md
Outdated
| A logged warning is expected for each deprecated item that is in deprecation window. | ||
|
|
||
| ## Version 1.11.0 (Pending) | ||
| * Use of `cluster`, found in [redis-proxy.proto](https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto) is deprecated. Set a `PrefixRoutes.catch_all_cluster` instead. |
Member
There was a problem hiding this comment.
Please merge master, this was moved into deprecated.rst. (You can now ref link also.)
docs/root/intro/version_history.rst
Outdated
| * ratelimit: removed deprecated rate limit configuration from bootstrap. | ||
| * redis: added :ref:`hashtagging <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.ConnPoolSettings.enable_hashtagging>` to guarantee a given key's upstream. | ||
| * redis: added :ref:`latency stats <config_network_filters_redis_proxy_per_command_stats>` for commands. | ||
| * redis: added :ref:`prefix routing <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.prefix_routes>` to enable routing commands based on their key's prefix to different upstream. |
Member
There was a problem hiding this comment.
Please move to this to the 1.11.0 section.
|
|
||
| bool MGETRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, | ||
| ConnPool::Instance* conn_pool) { | ||
| ConnPool::InstanceSharedPtr conn_pool) { |
Member
There was a problem hiding this comment.
nit: const ConnPool::InstanceSharedPtr&. Same elsewhere in this change.
added 6 commits
April 12, 2019 10:35
…uting-take-2 Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
…uting-take-2 Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
mattklein123
requested changes
Apr 15, 2019
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks looks great, just a few small things and please check the docs CI build.
/wait
docs/root/intro/version_history.rst
Outdated
| * ratelimit: removed deprecated rate limit configuration from bootstrap. | ||
| * redis: added :ref:`hashtagging <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.ConnPoolSettings.enable_hashtagging>` to guarantee a given key's upstream. | ||
| * redis: added :ref:`latency stats <config_network_filters_redis_proxy_per_command_stats>` for commands. | ||
| * redis: added :ref:`prefix routing <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.prefix_routes>` to enable routing commands based on their key's prefix to different upstream. |
|
|
||
| bool FragmentedRequest::onChildRedirection(const Common::Redis::RespValue& value, uint32_t index, | ||
| ConnPool::Instance* conn_pool) { | ||
| const ConnPool::InstanceSharedPtr conn_pool) { |
Member
There was a problem hiding this comment.
nit: const ConnPool::InstanceSharedPtr& same elsewhere where applicable.
| EXPECT_TRUE(fake_upstream_connection_2->close()); | ||
| } | ||
|
|
||
| // This test sends a simple |
Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
Signed-off-by: Maxime Bedard <maxime.bedard@shopify.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second take on #5658. During one of the iterations in the original PR, I actually forgot to short circuit when a route matches. I've updated the tests in 41d9b4d to confirm that they actually short circuit correctly.
I'm opening the PR, but @FAYiEKcbD0XFqF2QK2E4viAHg8rMm2VbjYKdjTg wanted to work on an integration test, I would wait for that integration test before merging this PR again.
Description: Add redis prefix routing.
Risk Level: Medium
Testing: Updated unit tests.
Docs Changes: N/A, revert of #5658.
Release Notes: N/A, revert of #5658.