mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <bbann...@apache.org>
Subject Re: Review Request 62353: Added a master-registry backed resource provider manager registry.
Date Thu, 21 Sep 2017 12:59:58 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message