mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 61528: Implemented a registrar for resource provider manager state.
Date Tue, 19 Sep 2017 04:30:58 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61528/#review183651
-----------------------------------------------------------




src/master/registry.proto
Lines 73-75 (patched)
<https://reviews.apache.org/r/61528/#comment259718>

    Hum, why we still need this?



src/master/registry.proto
Lines 73-75 (patched)
<https://reviews.apache.org/r/61528/#comment261935>

    Do we still need this?



src/resource_provider/registrar.hpp
Lines 83 (patched)
<https://reviews.apache.org/r/61528/#comment261954>

    Let's be more consistent. We tend not to use tailing underscore for member fields if no
needed (we do need if there is a public member accessor with the same name). Please fix all
that in this patch.



src/resource_provider/registrar.cpp
Lines 128-133 (patched)
<https://reviews.apache.org/r/61528/#comment261953>

    See my comments below. Probably don't need this if we tweek the protobuf.



src/resource_provider/registrar.cpp
Lines 190 (patched)
<https://reviews.apache.org/r/61528/#comment261957>

    I think we want to tie the lifecycle of resource providers to the lifecycle of agents
so that when the agent is gone, all the LRP associated with that agent is gone too.
    
    Given that we don't change agent ID upon slave machine reboot, i think it makes sense
to checkpoint the local resource provider information under `<work_dir>/slaves/<slave_id>/resource_provider_registry/`
    
    Let's introduce a helper in `src/slave/paths.hpp|cpp` for the path helper



src/resource_provider/registrar.cpp
Lines 308 (patched)
<https://reviews.apache.org/r/61528/#comment261958>

    Please fix all typos.



src/resource_provider/registrar.cpp
Lines 331 (patched)
<https://reviews.apache.org/r/61528/#comment261955>

    Given this is a c++ source file, i'll try to use 'using' clause in the begining and avoid
namespace prefix if possible.



src/resource_provider/registrar.cpp
Lines 337-338 (patched)
<https://reviews.apache.org/r/61528/#comment261956>

    Ditto.



src/resource_provider/registry.proto
Lines 42 (patched)
<https://reviews.apache.org/r/61528/#comment261952>

    hum, any reason we don't use:
    ```
    message Registry {
      repeated ResourceProvider resource_providers;
    }
    ```


- Jie Yu


On Sept. 15, 2017, 1:16 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61528/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2017, 1:16 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7555
>     https://issues.apache.org/jira/browse/MESOS-7555
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a registry and registrar interface for resource
> provider managers. The registrar interface is modelled after the
> master registrar and supports similar operations. Currently a single,
> LevelDB-backed registrar is implemented which we plan to use for
> resource provider managers in agents.
> 
> Current the registry allows to add and remove resource provider IDs.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
>   src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900 
>   src/master/registry.proto 362a9fab946e9fb0411df2277f7f3edbadccb61a 
>   src/resource_provider/registrar.hpp PRE-CREATION 
>   src/resource_provider/registrar.cpp PRE-CREATION 
>   src/resource_provider/registry.hpp PRE-CREATION 
>   src/resource_provider/registry.proto PRE-CREATION 
>   src/tests/resource_provider_manager_tests.cpp 3bc56b51526e9dd188423f7349e74246c3295c77

> 
> 
> Diff: https://reviews.apache.org/r/61528/diff/5/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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