-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Refactor TransportSocketFactoryContext and Cluster interfaces. #4026
Conversation
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>
/** | ||
* @return information about the local environment the server is running in. | ||
*/ | ||
virtual const LocalInfo::LocalInfo& local_info() PURE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localInfo
source/common/upstream/eds.h
Outdated
const LocalInfo::LocalInfo& local_info, ClusterManager& cm, | ||
Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, | ||
bool added_via_api); | ||
bool added_via_api, |
There was a problem hiding this comment.
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?
source/common/upstream/eds.h
Outdated
bool added_via_api); | ||
bool added_via_api, | ||
Server::Configuration::TransportSocketFactoryContext& factory_context, | ||
Stats::ScopePtr stats_scope); |
There was a problem hiding this comment.
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_; |
There was a problem hiding this comment.
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>
@lizan Please take a look. Thanks! |
EXPECT_THROW(StaticClusterImpl(parseClusterFromJson(json), runtime, stats, ssl_context_manager, | ||
local_info, cm, false), | ||
EnvoyException); | ||
EXPECT_THROW( |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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>
There was a problem hiding this 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. :)
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