-
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
test infra: Remove timeSource() from the ClusterManager api #4247
Changes from 28 commits
da7f24a
7d59de4
10422d4
282307f
fa5c8c8
8d67f55
c9a7d62
2d9f2aa
1a84061
eb2347e
21ac418
0a2514c
c642dbc
646a500
e65a754
50dbb33
be44f2d
1488d58
44d880d
6d9fac6
944d75b
28f41dd
b9a9cee
a285196
63fb553
4f309ab
7ea31e8
656d40c
200c70e
0a6c3b2
f3c825e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,26 +96,29 @@ class FilterConfig { | |
FilterConfig(const std::string& stat_prefix, const LocalInfo::LocalInfo& local_info, | ||
Stats::Scope& scope, Upstream::ClusterManager& cm, Runtime::Loader& runtime, | ||
Runtime::RandomGenerator& random, ShadowWriterPtr&& shadow_writer, | ||
bool emit_dynamic_stats, bool start_child_span, bool suppress_envoy_headers) | ||
bool emit_dynamic_stats, bool start_child_span, bool suppress_envoy_headers, | ||
TimeSource& time_source) | ||
: scope_(scope), local_info_(local_info), cm_(cm), runtime_(runtime), | ||
random_(random), stats_{ALL_ROUTER_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix))}, | ||
emit_dynamic_stats_(emit_dynamic_stats), start_child_span_(start_child_span), | ||
suppress_envoy_headers_(suppress_envoy_headers), shadow_writer_(std::move(shadow_writer)) {} | ||
suppress_envoy_headers_(suppress_envoy_headers), shadow_writer_(std::move(shadow_writer)), | ||
time_source_(time_source) {} | ||
|
||
FilterConfig(const std::string& stat_prefix, Server::Configuration::FactoryContext& context, | ||
ShadowWriterPtr&& shadow_writer, | ||
const envoy::config::filter::http::router::v2::Router& config) | ||
: FilterConfig(stat_prefix, context.localInfo(), context.scope(), context.clusterManager(), | ||
context.runtime(), context.random(), std::move(shadow_writer), | ||
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, dynamic_stats, true), | ||
config.start_child_span(), config.suppress_envoy_headers()) { | ||
config.start_child_span(), config.suppress_envoy_headers(), | ||
context.timeSource()) { | ||
for (const auto& upstream_log : config.upstream_log()) { | ||
upstream_logs_.push_back(AccessLog::AccessLogFactory::fromProto(upstream_log, context)); | ||
} | ||
} | ||
|
||
ShadowWriter& shadowWriter() { return *shadow_writer_; } | ||
TimeSource& timeSource() { return cm_.timeSource(); } | ||
TimeSource& timeSource() { return time_source_; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... and I guess here as well.. neither of these are new to this PR, so I think it makes sense to merge and continue the discussion elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed in source/common/router/router.cc:782, and there is no current 'friend' declaration, so I prefer to leave it as is; it's not clear why this exposes more than the shadowWriter accessor above. |
||
|
||
Stats::Scope& scope_; | ||
const LocalInfo::LocalInfo& local_info_; | ||
|
@@ -130,6 +133,7 @@ class FilterConfig { | |
|
||
private: | ||
ShadowWriterPtr shadow_writer_; | ||
TimeSource& time_source_; | ||
}; | ||
|
||
typedef std::shared_ptr<FilterConfig> FilterConfigSharedPtr; | ||
|
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.
Still a bit puzzled why we need to have a getter for
TimeSource
here..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.
source/common/grpc/async_client_impl.cc:211 needs this, but I see that class is already a friend, and in this code we already have the pattern of friends digging into private member vars without going thru a private method. So I can remove it.