Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor TransportSocketFactoryContext and Cluster interfaces. #4026

Merged

Conversation

JimmyCYJ
Copy link
Member

@JimmyCYJ JimmyCYJ commented Aug 1, 2018

Refactor TransportSocketFactoryContext and Cluster interfaces.

Description:
According to #3700 , we introduce TransportSocketFactoryContextImpl that simplifies the interfaces of
ListenerImpl, ClusterImplBase and its derived classes. This PR implements TransportSocketFactoryContextImpl, and refactors ListenerImpl and Cluster interfaces. This PR is split out of #3700.

Risk Level: Low
Signed-off-by: Jimmy Chen jimmychen.0102@gmail.com

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
@JimmyCYJ
Copy link
Member Author

JimmyCYJ commented Aug 1, 2018

@lizan @qiwzhang please take a look. Thanks!

@lizan lizan self-assigned this Aug 1, 2018
/**
* @return information about the local environment the server is running in.
*/
virtual const LocalInfo::LocalInfo& local_info() PURE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localInfo

const LocalInfo::LocalInfo& local_info, ClusterManager& cm,
Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random,
bool added_via_api);
bool added_via_api,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep added_via_api as the last parameter?

bool added_via_api);
bool added_via_api,
Server::Configuration::TransportSocketFactoryContext& factory_context,
Stats::ScopePtr stats_scope);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScopePtr&&? (also for other ClusterImpl)

Stats::IsolatedStoreImpl load_report_stats_store_;
mutable ClusterLoadReportStats load_report_stats_;
Network::TransportSocketFactoryPtr transport_socket_factory_;
Stats::ScopePtr stats_scope_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change the order?

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
@JimmyCYJ
Copy link
Member Author

JimmyCYJ commented Aug 2, 2018

@lizan Please take a look. Thanks!

EXPECT_THROW(StaticClusterImpl(parseClusterFromJson(json), runtime, stats, ssl_context_manager,
local_info, cm, false),
EnvoyException);
EXPECT_THROW(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're already here, make this EXPECT_THROW_WITH_MESSAGE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Please take a look.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like this cleanup. :)

@mattklein123 mattklein123 merged commit 9bc0472 into envoyproxy:master Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants