> On Sept. 20, 2017, 9:36 p.m., Jie Yu wrote:
> > src/resource_provider/registrar.hpp
> > Lines 133 (patched)
> > <https://reviews.apache.org/r/62353/diff/1/?file=1827904#file1827904line133>
> >
> > No need for `explicit` because this is a two arguments constructor
I made this a one-arg constructor, so we should make it `explicit` now.
As an aside, from on C++11 `explicit` carries meaning even for multi-arg
constructors, consider
MasterRegistrar foo(Registrar* registrar, const Registry& registry)
{
return {registrar, registry}; // Only works for non-explicit ctrs.
}
Since we use neither `explicit` multiarg constructors nor initializer lists in
return values this is not terribly relevant for Mesos at the moment.
> On Sept. 20, 2017, 9:36 p.m., Jie Yu wrote:
> > src/resource_provider/registrar.cpp
> > Lines 382 (patched)
> > <https://reviews.apache.org/r/62353/diff/1/?file=1827905#file1827905line382>
> >
> > Ditto. No need for `explicit`
Ditto.
> On Sept. 20, 2017, 9:36 p.m., Jie Yu wrote:
> > src/resource_provider/registrar.cpp
> > Lines 372-376 (patched)
> > <https://reviews.apache.org/r/62353/diff/2/?file=1829922#file1829922line372>
> >
> > This is definitely weird to me. I think we can get rid of the
> > dependency of taking an internal `internal::Registry& registry` by doing
> > the following:
> >
> > 1) Move `AdaptedOperation` to master registrar. I would call it
> > `UpdateResourceProviderRegistryOperation`.
> >
> > 2) `recover()` probably does not need to return `Registry`. If we get
> > rid of that, we probably don't need to expose the internal Registry.
Not returning a registry from `recover` seems to be sufficient to just work on
a master registrar so we do not need to pass the master registry anymore.
As for moving the adaptor operation to the master, I am not sure this is a good
idea (if I understand you correctly; I am assuming you mean moving it close to
the other master registrar operations, and not into `master/registrar.hpp`
which just defines the master registrar interface). It seems that would bleed
resource provider registry information into master code (we would e.g., need to
make at least the concept of resource provider registrar operations known
there). The only benefit I could see would be to collect all derived operations
into a single place; I explicitly used e.g., `override` here to minimize the
risk of divergence between the master operation interface and the interface
`AdaptedOperation` implements.
Please let me know if I misunderstood what you meant.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62353/#review185695
-----------------------------------------------------------
On Sept. 21, 2017, 2:59 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62353/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2017, 2:59 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds an implementation of the resource provider registrar
> backed by the master's registrar. With that it becomes possible to
> persist resource provider manager state in a single master registrar,
> but use the narrowly defined resource provider registry.
>
>
> Diffs
> -----
>
> src/master/registry.proto 362a9fab946e9fb0411df2277f7f3edbadccb61a
> src/resource_provider/registrar.hpp PRE-CREATION
> src/resource_provider/registrar.cpp PRE-CREATION
> src/tests/resource_provider_manager_tests.cpp
> 3bc56b51526e9dd188423f7349e74246c3295c77
>
>
> Diff: https://reviews.apache.org/r/62353/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
