SGTM. I would suggest to first address the non-proto files before changing
the proto files.
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/
>
