Hi Jie,
Thanks for the comments. I have tried to answer them inline. Please let us
know if something isn't clear.
Kapil
On Tue, Jan 27, 2015 at 1:25 AM, Jie Yu <yujie....@gmail.com> wrote:
> One benefit of having an internal namespace is that it tells the
> framework/executor writer that those symbols/method/class are internal to
> Mesos core and should not be used.
>
We don't have any internal symbols/methods/classes in public headers, do
we? This is assuming that a framework writer installs a mesos-dev package
or equivalent and doesn't deal with mesos source tree.
> If we kill all the internal namespaces and move many headers like
> isolator.hpp to include/mesos, how does the framework writer know that
> he/she shouldn't use some of the headers because they are internal to Mesos
> and are subject to change?
>
I do agree on your general point about exposing more files to framework
writers. However, shouldn't a framework writer be using the headers, etc.
based on their requirements rather than grabbing anything and everything
that is exposed as public headers?
>
> For modules, I am wondering can we separate Mesos public headers (in
> include/mesos right now) from those headers that are only for building
> modules (more like internal public headers).
>
It's a bit complicated. There are some files that correspond only to the
slave (i.e. mesos/slave/state.hpp, mesos/slave/isolator.hpp) and similarly,
very soon we'll have master-specific files as well. Similarly, there are
some shared files such as those authenticator/authenticatee, that are
shared by both master and slave. In all these cases, these files aren't
just about modules, instead, modules are only _one_ of the consumers of
these files.
Further, module-specific files currently reside in mesos/module/. As per
the current file layout, files in mesos/module/ #include files from mesos/.
For example, mesos/module/isolator.hpp #includes mesos/slave/isolator.hpp.
Is there an alternate file layout suggestion that we should think about?
> Thoughts?
>
> - Jie
>
> On Mon, Jan 26, 2015 at 12:35 PM, Kapil Arya <ka...@mesosphere.io> wrote:
>
> > Hi All,
> >
> > TLDR: We currently use "mesos::internal" namespace for almost everything
> > inside src/. However, in most cases, it is directly enclosing another
> > namespace. This makes the "internal" namespace redundant and we should
> > kill it.
> >
> > I learned from Ben Hindman that the original motivation for introducing
> an
> > explicit internal namespace was to discourage people from exposing
> internal
> > symbols through "extern", etc. Since we don't seem to expose symbols
> > through "extern" in our codebase, I think it's safe to kill this
> namespace.
> >
> > Here is a list of files that define classes directly in the
> mesos::internal
> > namespace (i.e. without enclosing a separate namespace) [1]:
> >
> > authorizer/authorizer.hpp
> > common/http.hpp
> > common/attributes.hpp
> > common/lock.hpp
> > files/files.hpp
> > hook/manager.hpp
> > master/contender.hpp
> > master/detector.hpp
> > usage/usage.hpp
> > watcher/whitelist_watcher.hpp
> >
> > messages/messages.pb.h
> >
> > Of the above list, things like hook/manager.hpp and
> > master/{contender,detector}.hpp can be moved into their own namespaces.
> I
> > am sure, we can come up with a strategy for the rest as well.
> >
> > One possible concern here might be messages.proto and the effects on
> > upgrades, etc., if we change the namespace/package for these protobufs.
> If
> > this turns out to be a real concern, we can possibly keep the internal
> > namespace for messages.proto.
> >
> > If we kill the "internal" namespace altogether, it would make it much
> > easier to expose some headers as public headers for modularization, etc..
> > This will also help us get rid of "namespace internal" from some of the
> > public headers that we already have.
> >
> > The motivation for killing the "internal" namespace comes from a
> > patch-set[2] that tries to expose slave/containerizer/isolator.hpp as
> > include/mesos/slave/isolator.hpp. We wanted to keep Isolator inside the
> > "mesos::slave" namespace instead of putting it directly in the "mesos"
> > namespace. This change causes a lot of conflicts due to "mesos::slave"
> and
> > "mesos::internal::slave" and we had to resort to using fully qualified
> > names for a large number of definition in src/slave, src/tests, etc.
> >
> > Best,
> > Kapil
> >
> > [1] List obtained by running the following command and then filtering out
> > the instances where "internal" was enclosed in something other than
> > "mesos":
> > grep -nr "} // namespace internal" * -B1 |grep -v
> > "namespace\|\-\-\|\.cpp"|cut -d'-' -f1
> >
> > [2] https://reviews.apache.org/r/30238/
> > https://reviews.apache.org/r/29602/
> > https://reviews.apache.org/r/29603/
> >
>
