api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places. by jmarantz · Pull Request #5660 · envoyproxy/envoy

jmarantz · 2019-01-18T22:48:05Z

Description: It's better to tunnel more global structures through the API interface rather than passing tons of of global state everywhere, as it makes it much easier to add state as needed, such as Filesystem::Instance (@sesmith177) or Stats::SymbolTable in the futre. This PR puts an Event::TimeSystem& accessor into Api::Api and plumbs that through where needed.

Risk Level: low this is a straight refactor.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

…in various places. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

jmarantz · 2019-01-19T20:50:43Z

hold off on review; I found a way to simplify this.

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

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

jmarantz · 2019-01-22T16:50:48Z

I think this is probably too big to review but I'd love to get any feedback anyone has. My thinking is that there are 2-3 PRs to split out here:

Use Global to ensure there's only one time-system in tests, but that are reset on every test method.
Carry TimeSystem& around in API.
Remove all uses of DeprecatedDangerousTestTime.

I'm also thinking of another grand test-refactor which is make most non-trivial tests in Envoy derive from a single class. This class would also be a good place to assert that all Global are cleaned up, which is currently only done at the end of all tests.

sesmith177 · 2019-01-22T21:58:36Z

When passing the Filesystem::Instance& through the API, we found that we'd have to expose the API from the FactoryContext as well, so there is some overlap there (at least for the carrying the TimeSystem& around in the API) part.

We also gave the "all tests that use threads/filesystem derive from a single class" pattern a look -- as one might guess it turned into a pretty enormous change so we decided to put it down.

mattklein123 · 2019-01-23T03:00:03Z

@jmarantz SGTM. In general I do like the idea of carrying around Api everywhere as ultimately we are going to have to do this so we can have a stable API for extensions. I.e., from Api you can get an HttpApi and from there create a header map, etc. so I think this is super valuable work.

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

dnoe · 2019-01-28T18:17:06Z

I am very much in favor of using Api::Api for this, I think it's the best of all options for these global items like time and filesystem.

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

Still failing: //test/common/network:connection_impl_test FAILED in 2.2s [ RUN ] IpVersions/ConnectionImplDeathTest.BadFd/IPv4 test/common/network/connection_impl_test.cc:91: Failure Death test: ConnectionImpl(dispatcher, std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr), Network::Test::createRawBufferSocket(), false) Result: died but not with expected error. Expected: contains regular expression ".*assert failure: ioHandle\\(\\).fd\\(\\) != -1.*" Actual msg: Will investigate after master-merge. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

jmarantz · 2019-01-29T23:22:21Z

@sesmith177 @dnoe I think this is ready for a look.

jmarantz · 2019-01-30T01:28:34Z

/retest

repokitteh-read-only · 2019-01-30T01:28:37Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #5660 (comment) was created by @jmarantz.

see: more, trace.

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

jmarantz · 2019-01-31T01:07:40Z

/retest

repokitteh-read-only · 2019-01-31T01:07:44Z

🔨 rebuilding ci/circleci: mac (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #5660 (comment) was created by @jmarantz.

see: more, trace.

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

dnoe

I think you can remove an argument from the GuardDogImpl constructor, since it currently takes both a TimeSystem and an Api:

https://github.com/envoyproxy/envoy/blob/master/source/server/guarddog_impl.h#L43

source/common/api/api_impl.h

source/server/config_validation/server.cc

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

…th explicit TimeSystem arg. Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 · 2019-02-03T22:38:30Z

include/envoy/api/api.h

+  /**
+   * @return a reference to the TimeSystem
+   */
+  virtual Event::TimeSystem& timeSystem() PURE;


Per discussion, WDYT about just making this return a TimeSource in this PR? We can still take a TimeSystem in the constructor and not change anything else until a follow up, but this would get us closer to where we want to be?

I think that would expand this PR significantly because there are a few places in the PR where an existing timeSystem() interface is implemented using API::timeSystem(). If that disappeared I'd have to change a bunch of call-sites. This would be trivial but it would make the PR a lot larger, and there is some semantic content here. So I'd prefer to do that in a separate PR. That OK?

I have this practice of trying to make semantically interesting PRs as small as possible, and letting trivial renaming or type-change PRs go large if needed.

Sure SGTM. I think we are on the same page and heading in the right direction so 👍

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

mattklein123

Awesome cleanup!

…stem in various places. (envoyproxy#5660) * Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

jmarantz added 2 commits January 18, 2019 17:43

Put Event::TimeSystem& into API and plumb that instead of TimeSystem …

3f92f52

…in various places. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove TimeSystem arg from DispatcherImpl.

991f7e6

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

jmarantz changed the title ~~WiP api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places.~~ api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places. Jan 19, 2019

jmarantz requested a review from sesmith177 January 19, 2019 02:58

jmarantz mentioned this pull request Jan 19, 2019

filesystem: add Windows implementation #5470

Closed

jmarantz changed the title ~~api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places.~~ WiP api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places. Jan 19, 2019

jmarantz changed the title ~~WiP api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places.~~ api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places. Jan 19, 2019

jmarantz added 6 commits January 20, 2019 23:39

use a Global to manage time-systems to avoid having to plumb them thr…

b9add27

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

Remove TimeSystem argument to createApiForTest.

53b83d9

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

format

a1234d7

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

Fix more compiler issues.

f256e6c

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

Simplifications.

d1ba8e6

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

remove extra include.

d5dbd0f

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

jmarantz changed the title ~~api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places.~~ RFC: api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places. Jan 22, 2019

jmarantz added 2 commits January 22, 2019 22:21

some delegation hacks to reduce the amount of indirection.

dbc6f3b

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

format

3f7396e

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

sesmith177 mentioned this pull request Jan 23, 2019

filesystem: convert free functions to object methods #5692

Merged

jmarantz mentioned this pull request Jan 25, 2019

test: Make time-system into a global singleton to simplify its use and plumbing, and avoid multi-time-system test confusion. #5708

Merged

mattklein123 assigned dnoe Jan 28, 2019

jmarantz mentioned this pull request Jan 28, 2019

thread: thread-factory singleton assertion #5734

Closed

jmarantz added 5 commits January 28, 2019 23:20

Merge branch 'master' into plumb-time-source-thru-api

0deb25b

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

Merge branch 'master' into plumb-time-source-thru-api

f4088b6

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

remove non-essential change.

5fad5d6

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

More diff reduction.

98fe7cc

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

jmarantz changed the title ~~RFC: api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places.~~ api: Put Event::TimeSystem& into API and plumb that instead of TimeSystem in various places. Jan 29, 2019

jmarantz added 4 commits January 29, 2019 22:25

Merge branch 'master' into plumb-time-source-thru-api

6866490

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

Merge branch 'master' into plumb-time-source-thru-api

9605da6

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

Update API instantiation reference

cf51895

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

Merge branch 'master' into plumb-time-source-thru-api

741a021

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

Merge branch 'master' into plumb-time-source-thru-api

eec742d

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

dnoe reviewed Jan 31, 2019

View reviewed changes

source/common/api/api_impl.h Show resolved Hide resolved

source/server/config_validation/server.cc Show resolved Hide resolved

jmarantz added 3 commits January 31, 2019 16:02

Merge branch 'master' into plumb-time-source-thru-api

ca1c0a8

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

make API accessors consistently inlined

53394d6

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

Address reivew comments, and provide a variant of createApiForTest wi…

b08d473

…th explicit TimeSystem arg. Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 reviewed Feb 3, 2019

View reviewed changes

mattklein123 self-assigned this Feb 3, 2019

mattklein123 added the waiting label Feb 3, 2019

jmarantz added 2 commits February 4, 2019 09:15

Merge branch 'master' into plumb-time-source-thru-api

8e3ccb6

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

Merge branch 'master' into plumb-time-source-thru-api

f9ea8eb

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

repokitteh-read-only bot removed the waiting label Feb 4, 2019

mattklein123 added the waiting label Feb 4, 2019

Remove superfluous test_time_. Make test-name unique.

1aae0cc

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

repokitteh-read-only bot removed the waiting label Feb 5, 2019

jmarantz added 2 commits February 5, 2019 08:17

Merge branch 'master' into plumb-time-source-thru-api

b4817b2

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

Add TODO for changing API interface to TimeSource.

2b5a16e

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

mattklein123 approved these changes Feb 5, 2019

View reviewed changes

jmarantz merged commit ed05163 into envoyproxy:master Feb 5, 2019

jmarantz deleted the plumb-time-source-thru-api branch February 5, 2019 17:27

Conversation

jmarantz commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Jan 19, 2019

Uh oh!

jmarantz commented Jan 22, 2019

Uh oh!

sesmith177 commented Jan 22, 2019

Uh oh!

mattklein123 commented Jan 23, 2019

Uh oh!

dnoe commented Jan 28, 2019

Uh oh!

jmarantz commented Jan 29, 2019

Uh oh!

jmarantz commented Jan 30, 2019

Uh oh!

repokitteh-read-only bot commented Jan 30, 2019

Uh oh!

jmarantz commented Jan 31, 2019

Uh oh!

repokitteh-read-only bot commented Jan 31, 2019

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattklein123 Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarantz commented Jan 18, 2019 •

edited

Loading