Thanks James! As you said, removing Process implementations from the headers is the existing practice, but we need to do a sweep to enforce this consistently. Folks could work on this sweep today to make progress on the 3 benefits you outlined.
This proposal to me seems to just be: (1) When needed for testing, whether to expose the Process declaration in its own foo_process.hpp header, rather than within foo.hpp. (2) whether to name the .cpp as foo_process.cpp rather than foo.cpp. I'm not sure if I like (2), instead of keeping the .cpp named foo.cpp. Consider the case where there is no foo_process.hpp (not needed for testing), then you just have foo.hpp and foo_process.cpp. Or consider the case where a user is looking for the implementation of limiter.hpp, they have to know to look for limiter_process.cpp rather than limiter.cpp (but only when a Process is involved!). Seems unfortunate? For Mesos, (1) sounds good, but I'm not sure if libprocess should be exposing the foo_process.hpp header in the public includes alongside the foo.hpp header. Because then libprocess users are assuming our particular implementation of the interface. I think for the libprocess testing purposes, we probably want the *_process.hpp header to be within the src/ directory? On Sat, Jun 24, 2017 at 8:23 AM, James Peach <jor...@gmail.com> wrote: > Hi all, > > There is a common Mesos pattern where a subsystem is implemented by a > facade class that forwards calls to an internal Process class, eg. Fetcher > and FetcherProcess, or zookeeper::Group and zookeeper::GroupProcess. Since > the Process is an internal implementation detail, I'd like to propose that > we adopt a general policy that it should not be exposed in the primary > header file. This has the following benefits: > > - reduces the number of symbols exposed to clients including the primary > header file > - reduces the number of header files needed in the primary header file > - reduces the number of rebuilt dependencies when the process > implementation changes > > Although each individual case of this practice may not improve build > times, I think it is likely that over time, consistent application of this > will help. > > In many cases, when FooProcess is only used by Foo, both the declaration > and definitions of Foo can be inlined into "foo.cpp", which is already our > common practice. If the implementation of the Process class is needed > outside the facade (eg. for testing), the pattern I would propose is: > > foo.hpp - Primary API for Foo, forward declares FooProcess > foo_process.hpp - Declarations for FooProcess > foo_process.cpp - Definitions of FooProcess > > The "checks/checker.hpp" interface almost follows this pattern, but gives > up the build benefits by including "checker_process.hpp" in "checker.hpp". > This should be simple to fix however. > > thanks, > James
