Re: RFC: removing process implementations from common headers

[Benjamin Mahler]

Sounds good to me, I also like putting the internal limiter_process.hpp (if
needed for testing) in the src folder (note that we already have internal
headers there).

Thanks James! Looking forward to having this tidied up. Let me know if you
need any assistance wrangling reviewers :)

On Wed, Jun 28, 2017 at 8:54 PM James Peach  wrote:

>
> > On Jun 28, 2017, at 2:19 AM, Benjamin Mahler  wrote:
> >
> > 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?
>
> I'm OK with putting both Foo and FooProcess in foo.cpp.
>
> > 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?
>
> There are 2 options for libprocess. Put the internal headers in the src/
> directory, or keep them in include/ but don't install them (use
> noinst_HEADERS). The former gives better protection against accidentally
> consuming the internal headers from Mesos.
>
> > On Sat, Jun 24, 2017 at 8:23 AM, James Peach  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
>
>

Re: RFC: removing process implementations from common headers

[James Peach]

> On Jun 28, 2017, at 2:19 AM, Benjamin Mahler  wrote:
> 
> 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?

I'm OK with putting both Foo and FooProcess in foo.cpp.

> 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?

There are 2 options for libprocess. Put the internal headers in the src/ 
directory, or keep them in include/ but don't install them (use 
noinst_HEADERS). The former gives better protection against accidentally 
consuming the internal headers from Mesos.

> On Sat, Jun 24, 2017 at 8:23 AM, James Peach  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

Re: RFC: removing process implementations from common headers

[Benjamin Mahler]

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  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

Re: RFC: removing process implementations from common headers

[Alex Rukletsov]

I'm in favor of the suggestion. Do you guys plan to do a single sweep or
document the pattern somewhere and apply it only for new and refactored
code?

On Wed, Jun 28, 2017 at 12:19 AM, Yan Xu  wrote:

> This sounds reasonable to me. Do others have comments?
>
> ---
> @xujyan 
>
> On Fri, Jun 23, 2017 at 4:23 PM, James Peach  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
>

Re: RFC: removing process implementations from common headers

[Yan Xu]

This sounds reasonable to me. Do others have comments?

---
@xujyan 

On Fri, Jun 23, 2017 at 4:23 PM, James Peach  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

RFC: removing process implementations from common headers

[James Peach]

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