----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48371/#review139416 -----------------------------------------------------------
Fix it, then Ship it! Great, thanks! I did some re-working of the patch to make the allocator a shared object and to make the implementation of the allocator process more succinct. Let me know if you see any issues. src/slave/containerizer/mesos/isolators/gpu/allocator.hpp (lines 86 - 111) <https://reviews.apache.org/r/48371/#comment204643> It seems a bit misleading that the allocation / deallocation methods are const. To avoid the need for making them const, returning a pointer and using process::Shared in the caller, we can make this a shared object, like process::Queue, process::http::Connection, process::Future, etc. src/slave/containerizer/mesos/isolators/gpu/allocator.hpp (line 100) <https://reviews.apache.org/r/48371/#comment204645> Hm.. None seems equivalent to Some empty set? Why distinguish these two cases? src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 200 - 208) <https://reviews.apache.org/r/48371/#comment204644> We should include the context in the error messages (e.g. failed to get the handle or get the minor number). src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 402 - 403) <https://reviews.apache.org/r/48371/#comment204648> Thanks for the test! src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 417 - 419) <https://reviews.apache.org/r/48371/#comment204646> Should we be checking that the gpu allocator agrees with this total? ``` ASSERT_EQ(total.get(), allocator->total().size()); ``` src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 442 - 474) <https://reviews.apache.org/r/48371/#comment204647> No need for all of these result variables, we can `AWAIT_READY(f())` directly. - Benjamin Mahler On June 23, 2016, 4:33 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48371/ > ----------------------------------------------------------- > > (Updated June 23, 2016, 4:33 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-5557 > https://issues.apache.org/jira/browse/MESOS-5557 > > > Repository: mesos > > > Description > ------- > > This component is designed to serve as a centralized component for > allocating / deallocating Nvidia GPUs. The goal is to share an > instance of this across both the mesos containerizer and the docker > containerizer so they can allocate GPUs from the same shared pool. > > It is overloaded to also do resource enumeration of GPUs based on > the agent flags. This keeps all code for enumerating GPUs and the > resources they represent in a single centralized location. > > > Diffs > ----- > > src/Makefile.am 23cac95a805e46d216e9479fea09f2c1619c45a5 > src/slave/containerizer/mesos/isolators/gpu/allocator.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp PRE-CREATION > src/tests/containerizer/nvidia_gpu_isolator_tests.cpp > a01c88660bb3c5435802ebd1cbc7ef84323c5795 > > Diff: https://reviews.apache.org/r/48371/diff/ > > > Testing > ------- > > GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests > > > Thanks, > > Kevin Klues > >
