rds: make RouteConfigProvider unique_ptr by lizan · Pull Request #3967 · envoyproxy/envoy

lizan · 2018-07-27T06:41:25Z

Signed-off-by: Lizan Zhou zlizan@google.com

Description:
RouteConfigProvider is no longer shared ownership, make it unique_ptr.
Follow up of #3960.

Risk Level: Low
Testing: unit test, integration tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Lizan Zhou <zlizan@google.com>

mattklein123

Thanks, awesome cleanup.

mattklein123 · 2018-07-27T15:48:38Z

include/envoy/router/route_config_provider_manager.h

   * dynamic (RDS) RouteConfigProviders currently loaded.
   */
-  virtual std::vector<RouteConfigProviderSharedPtr> getRdsRouteConfigProviders() PURE;
+  virtual std::vector<RouteConfigProvider*> getRdsRouteConfigProviders() PURE;


nit: Can we potentially return std::reference_wrappers here and below? Since these can never be null it would be a bit cleaner.

It is better to return list of RdsSubscription which is what the manager is holding. This function is only used for dumping RDS config. RdsSubscription is good enough

RdsSubscription is not implementing the RouteConfigProvider interface and there is no corresponding interface.

Do we really need these two methods (get{Rds,Static}RouteConfigProviders) in RouteConfigProviderManager interface? It is only used in rds_impl_test and RouteConfigProviderManagerImpl itself to dump config. cc @jsedgwick @htuch

I don't recall the entire history here. It's possible the methods are not needed anymore with iterative changes. Feel free to remove them if it's possible.

+1, remove any methods from the interface that don't have external callers.

- add more tests - make dump config public Signed-off-by: Lizan Zhou <zlizan@google.com>

qiwzhang · 2018-07-30T22:46:05Z

source/common/router/rds_impl.cc

+      dynamic_config->set_version_info(subscription->config_info_.value().last_config_version_);
+      dynamic_config->mutable_route_config()->MergeFrom(subscription->route_config_proto_);
    }
+    TimestampUtil::systemClockToTimestamp(subscription->last_updated_,


if subscription did not receive its first config (config_info_ is empty case), I think we don't need to create a config_dump entry.

I did this change intentionally as you want to know the RDS config is delivered but not received route from RDS. And it is easier to track how many RDS subscription Envoy has from config dump, thoughs?

OK, it will be useful to know there is such subscriptions that did not even receive the first responses.

Are you sure last_updated_ actually points to something meaningful here? Is it going to be 0 or the equivalent epoch? I guess I'm asking what will this actually output. :)

My preference would actually be to do this change in a follow up as a discrete change, and we should be consistent across all the xDS? Thoughts?

+1, prefer behavioral changes like this to be split out.

It will be initialized when the subscription started:

envoy/source/common/router/rds_impl.cc

Line 60 in 028387a

last_updated_(factory_context.systemTimeSource().currentTime()) {

Sure, I will make a separate PR.

I reverted this part for this PR.

qiwzhang · 2018-07-30T22:51:44Z

test/common/router/rds_impl_test.cc

-      route_config_provider_manager_->getRdsRouteConfigProviders();
-  EXPECT_EQ(2UL, configured_providers.size());
+  EXPECT_EQ(2UL,
+            route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size());


We only have one subscription, how can we have 2 dynamic route configs? Should it be 1?

we have two RDS config, rds_ and rds2, they are keyed by full Rds message, not only the name, they have different cluster.

qiwzhang · 2018-07-30T23:52:47Z

test/mocks/router/mocks.h

+  MOCK_METHOD2(createStaticRouteConfigProvider,
+               RouteConfigProviderPtr(const envoy::api::v2::RouteConfiguration& route_config,
+                                      Server::Configuration::FactoryContext& factory_context));
+  MOCK_METHOD0(getRdsRouteConfigProviders, std::vector<RouteConfigProvider*>());


why these two functions are still in the mock? I thought they are removed.

…rs() Signed-off-by: Lizan Zhou <zlizan@google.com>

…ider_unique

mattklein123

Awesome cleanup, thanks. One question.

mattklein123 · 2018-07-31T02:53:17Z

source/common/router/rds_impl.cc

+      dynamic_config->set_version_info(subscription->config_info_.value().last_config_version_);
+      dynamic_config->mutable_route_config()->MergeFrom(subscription->route_config_proto_);
    }
+    TimestampUtil::systemClockToTimestamp(subscription->last_updated_,


Are you sure last_updated_ actually points to something meaningful here? Is it going to be 0 or the equivalent epoch? I guess I'm asking what will this actually output. :)

My preference would actually be to do this change in a follow up as a discrete change, and we should be consistent across all the xDS? Thoughts?

htuch

LGTM modulo comment.

htuch · 2018-07-31T03:14:58Z

source/common/router/rds_impl.cc

+      dynamic_config->set_version_info(subscription->config_info_.value().last_config_version_);
+      dynamic_config->mutable_route_config()->MergeFrom(subscription->route_config_proto_);
    }
+    TimestampUtil::systemClockToTimestamp(subscription->last_updated_,


+1, prefer behavioral changes like this to be split out.

Signed-off-by: Lizan Zhou <zlizan@google.com>

mattklein123 · 2018-07-31T15:59:41Z

@lizan LGTM but needs format fix.

Signed-off-by: Lizan Zhou <zlizan@google.com>

…ider_unique

rds: make RouteConfigProvider unique_ptr

bda352e

Signed-off-by: Lizan Zhou <zlizan@google.com>

mattklein123 reviewed Jul 27, 2018

View reviewed changes

mattklein123 self-assigned this Jul 30, 2018

remove unused interface methods

140f85f

- add more tests - make dump config public Signed-off-by: Lizan Zhou <zlizan@google.com>

qiwzhang reviewed Jul 30, 2018

View reviewed changes

lizan added 2 commits July 30, 2018 16:57

clear unused getRdsRouteConfigProviders()/getStaticRouteConfigProvide…

b7ef2e5

…rs() Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into route_config_prov…

564373c

…ider_unique

qiwzhang previously approved these changes Jul 31, 2018

View reviewed changes

mattklein123 reviewed Jul 31, 2018

View reviewed changes

htuch reviewed Jul 31, 2018

View reviewed changes

not dump empty config

b3ca933

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan dismissed qiwzhang’s stale review via b3ca933 July 31, 2018 06:44

htuch previously approved these changes Jul 31, 2018

View reviewed changes

lizan added 2 commits July 31, 2018 09:52

fix format

7e80ee0

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into route_config_prov…

9ff8c50

…ider_unique

lizan dismissed htuch’s stale review via 9ff8c50 July 31, 2018 16:55

mattklein123 approved these changes Jul 31, 2018

View reviewed changes

mattklein123 merged commit 2012c3e into envoyproxy:master Jul 31, 2018

AndresGuedez mentioned this pull request Dec 14, 2018

config: Scoped RDS (PR 1/4): Introduce {static,dynamic} config provider framework #5243

Merged

Conversation

lizan commented Jul 27, 2018

Uh oh!

mattklein123 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!

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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants