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