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

Separate out Envoy platform API from extension API #5126

Open
htuch opened this issue Nov 26, 2018 · 4 comments
Open

Separate out Envoy platform API from extension API #5126

htuch opened this issue Nov 26, 2018 · 4 comments
Labels
design proposal Needs design doc/proposal before implementation help wanted Needs help!

Comments

@htuch
Copy link
Member

htuch commented Nov 26, 2018

We currently have a bit of conflation in api/ between those APIs that are provided a platform abstraction layer (mostly for the benefit of core Envoy, but also extensions) and the APIs that will one day act as a stable interface for extensions. For example:

virtual Event::DispatcherPtr allocateDispatcher(Event::TimeSystem& time_system) PURE;

is clearly in the second category, and

virtual Thread::ThreadPtr createThread(std::function<void()> thread_routine) PURE;

is in the first. This becomes even stranger when we have the ApiImpl require access to a stats object in

Stats::Store& stats_store);

This is because the filesystem code is both a platform abstraction API and also providing higher level functionality like stats tracking.

Ideally, we split out the API into a api/platform and api/export, where we clearly delineate these two concerns. New platform ports of Envoy populate the interfaces in api/platform, extensions take dependencies on api/export.

CC @jmarantz @eziskind @mattklein123

@htuch htuch added design proposal Needs design doc/proposal before implementation help wanted Needs help! labels Nov 26, 2018
@mattklein123
Copy link
Member

+1 from me. Also related to #827 and #3390.

@jmarantz
Copy link
Contributor

jmarantz commented Nov 27, 2018

I didn't realize the purpose of the API was just to provide a stable interface to extensions; I thought it also encapsulated platform functions on behalf of core code as well. If that's true should we think of a slightly different name from api/export?

But +1 on separating the platform API from the additions to Envoy. In the past I've used some variation on the decorator pattern. So for Filesystem we'd have:

Filesystem: abstract base class
FilesystemImpl: wrapper providing OO wrapper around open/close/read/write
FlushingFilesystem: ctor takes a Filesystem& and a ThreadFactory& and adds periodic flushing
StatsFilesystem: takes a Filesystem& and Stats::Store& and adds stat-tracking.

The FilesytemImpl would be contained as part of the PlatformAPI object. Alternate FilesystemImpl implementations should be straightforward to plug in (e.g. that wrap the Windows API for file i/o or provide a pure in-memory filesystem for tests).

The aggregated flushing/stats-taking filesystem would be contained as part of the ExportAPI object.

Similarly I think it might be interesting to take stats on threads, in which case we'd want to do it in a decorator class rather than in ThreadFactoryImpl -- which will have multiple alternate impls (eg Windows, Posix)

@jmarantz
Copy link
Contributor

jmarantz commented Nov 27, 2018

Also I would propose we move TimeSystem/TimeSource into API. Many places where TImeSystem& or TimeSource& is plumbed through, API& is as well, and it would seem better to use API as a conduit.

@jmarantz
Copy link
Contributor

jmarantz commented Dec 7, 2018

@jstordeur fyi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design proposal Needs design doc/proposal before implementation help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

3 participants