time: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate #4343

jmarantz · 2018-09-05T11:21:07Z

Description: In #4257, Event::TimeSystem was introduced to augment the TimeSource abstraction to include timers. Most places in the system that had previously held onto a TimeSource held it in a member variable time_source_ and, if appropriate, exposed it as a method timeSource(), and #4257 changed those to Event::TimeSystem without renaming them, in order to minimize diffs. This PR just renames the members and methods to time_system_ and timeSystem() as appropriate. This is part of the chain of CLs to resolve #4160.

Risk Level: low -- no functional risk, but depending on timing of when this gets merged, an easily-remedied build breakage could arise.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-09-05T11:27:30Z

This PR is has only mechanical renames, no re-organization or functional changes occurred.

dnoe · 2018-09-05T18:54:14Z

Are all three of your time-related PRs independent or is there any desired order of review/merging that will make people's lives easier?

jmarantz · 2018-09-05T19:06:18Z

The 3 are independent, especially the one that adds simulated time. This one can be reviewed either before or after #4341 but after one is merged, I believe the other one will need a minor fix before it can be merged.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-09-06T14:23:55Z

It would be good to review this one soon if possible; the sooner it gets in, the less time dealing with merge conflicts. It's entirely mechanical.

jmarantz · 2018-09-06T20:15:33Z

@htuch PTAL so you don't forget :)

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

@jmarantz it looks like there is still a fair bit of type/name mismatch in this PR; in some places it is introducing new sites where the name/type don't align.

include/envoy/server/filter_config.h

include/envoy/upstream/cluster_manager.h

source/common/common/token_bucket_impl.h

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-09-07T02:31:08Z

Sorry about that; I had not scrutinized the diffs enough after sedding. Now I've made sure all declarations are consistent with variable names. Note that we still in places initialize a TimeSource& with a Event::TimeSystem&, which is fine as Event::TimeSystem inherits from TimeSource.

htuch · 2018-09-07T15:32:15Z

source/common/config/grpc_mux_impl.cc

@@ -14,7 +14,7 @@ GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::AsyncClie
                         const Protobuf::MethodDescriptor& service_method,
                         Runtime::RandomGenerator& random)
    : local_info_(local_info), async_client_(std::move(async_client)),
-      service_method_(service_method), random_(random), time_source_(dispatcher.timeSource()) {
+      service_method_(service_method), random_(random), time_source_(dispatcher.timeSystem()) {


This one seems to still not match.

WAI. The dispatcher keeps a TimeSystem because it works with timers. However, gRPC doesn't care about timers (yet) so it needs only a TimeSource. So when testing this, all you need is (say) a MockTimeSource, and don't need to set up a whole TimeSystem.

This is true with all the cases below as well.

htuch · 2018-09-07T15:32:38Z

source/common/grpc/google_async_client_impl.h

@@ -165,7 +165,7 @@ class GoogleAsyncClientImpl final : public AsyncClient, Logger::Loggable<Logger:
  AsyncStream* start(const Protobuf::MethodDescriptor& service_method,
                     AsyncStreamCallbacks& callbacks) override;

-  TimeSource& timeSource() { return dispatcher_.timeSource(); }
+  TimeSource& timeSource() { return dispatcher_.timeSystem(); }


htuch · 2018-09-07T15:32:57Z

source/common/upstream/cluster_manager_impl.cc

@@ -180,7 +180,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
      init_helper_([this](Cluster& cluster) { onClusterInit(cluster); }),
      config_tracker_entry_(
          admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })),
-      time_source_(main_thread_dispatcher.timeSource()), dispatcher_(main_thread_dispatcher) {
+      time_source_(main_thread_dispatcher.timeSystem()), dispatcher_(main_thread_dispatcher) {


htuch · 2018-09-07T15:33:14Z

source/common/upstream/load_stats_reporter.cc

@@ -16,7 +16,7 @@ LoadStatsReporter::LoadStatsReporter(const LocalInfo::LocalInfo& local_info,
      async_client_(std::move(async_client)),
      service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName(
          "envoy.service.load_stats.v2.LoadReportingService.StreamLoadStats")),
-      time_source_(dispatcher.timeSource()) {
+      time_source_(dispatcher.timeSystem()) {


.. and more :)

htuch

Thanks!

jmarantz added 3 commits September 4, 2018 23:18

time_source_ -> time_system_ in most places.

362bebb

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Rename 'timeSource()' to 'timeSystem()'

f87fa85

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove TODOs that are done.

3a96c34

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~toime: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate~~ time: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate Sep 5, 2018

jmarantz added 2 commits September 6, 2018 07:39

Merge branch 'master' into time-source-to-system

c555ccc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Apply changes intended for this PR to newly merged envoyproxy#4288

144864e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into time-source-to-system

fbde0e8

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch suggested changes Sep 6, 2018

View reviewed changes

include/envoy/server/filter_config.h Outdated Show resolved Hide resolved

include/envoy/upstream/cluster_manager.h Outdated Show resolved Hide resolved

source/common/common/token_bucket_impl.h Outdated Show resolved Hide resolved

htuch self-assigned this Sep 6, 2018

make naming consistent with types.

dcef91f

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch reviewed Sep 7, 2018

View reviewed changes

htuch approved these changes Sep 7, 2018

View reviewed changes

htuch merged commit c886007 into envoyproxy:master Sep 7, 2018

jmarantz deleted the time-source-to-system branch September 7, 2018 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

time: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate #4343

time: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate #4343

jmarantz commented Sep 5, 2018 •

edited

Loading

jmarantz commented Sep 5, 2018

dnoe commented Sep 5, 2018

jmarantz commented Sep 5, 2018

jmarantz commented Sep 6, 2018

jmarantz commented Sep 6, 2018

htuch left a comment

jmarantz commented Sep 7, 2018

htuch Sep 7, 2018

jmarantz Sep 7, 2018

htuch Sep 7, 2018

htuch Sep 7, 2018

htuch Sep 7, 2018

htuch left a comment

time: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate #4343

time: Rename time_source_ and timeSource() to time_system_ and timeSystem() as appropriate #4343

Conversation

jmarantz commented Sep 5, 2018 • edited Loading

jmarantz commented Sep 5, 2018

dnoe commented Sep 5, 2018

jmarantz commented Sep 5, 2018

jmarantz commented Sep 6, 2018

jmarantz commented Sep 6, 2018

htuch left a comment

Choose a reason for hiding this comment

jmarantz commented Sep 7, 2018

htuch Sep 7, 2018

Choose a reason for hiding this comment

jmarantz Sep 7, 2018

Choose a reason for hiding this comment

htuch Sep 7, 2018

Choose a reason for hiding this comment

htuch Sep 7, 2018

Choose a reason for hiding this comment

htuch Sep 7, 2018

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

jmarantz commented Sep 5, 2018 •

edited

Loading