----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49615/#review140885 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 381 - 384) <https://reviews.apache.org/r/49615/#comment206224> Why duplicate the comment from the header in the .cpp file? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 387 - 393) <https://reviews.apache.org/r/49615/#comment206225> You don't need either of these checks src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 395) <https://reviews.apache.org/r/49615/#comment206226> why auto instead of `const Label&`? src/slave/containerizer/mesos/isolators/gpu/volume.cpp (line 396) <https://reviews.apache.org/r/49615/#comment206227> You don't need the has_key check here src/slave/containerizer/mesos/isolators/gpu/volume.cpp (lines 397 - 399) <https://reviews.apache.org/r/49615/#comment206228> I'm a bit confused by the comment, what is it used for exactly? As discussed, let's say that it is used as the volume name, which is unnecessary for us because we use the host path directly. src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 488 - 512) <https://reviews.apache.org/r/49615/#comment206231> Seems we also need a false case here? - Benjamin Mahler On July 4, 2016, 10:09 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49615/ > ----------------------------------------------------------- > > (Updated July 4, 2016, 10:09 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-5401 > https://issues.apache.org/jira/browse/MESOS-5401 > > > Repository: mesos > > > Description > ------- > > We use the the `com.nvidia.volumes.needed` label from > nvidia-docker to decide if we should inject the volume or not: > > https://github.com/NVIDIA/nvidia-docker/wiki/Image-inspection > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/gpu/volume.cpp > 9aac3cc8622c21014de87576f84180d30ae3fadd > src/tests/containerizer/nvidia_gpu_isolator_tests.cpp > 474040c89d69b50c051122d873c11a102b33c538 > > Diff: https://reviews.apache.org/r/49615/diff/ > > > Testing > ------- > > `GTEST_FILTER="" make -j check && > GTEST_FILTER="*NVIDIA_GPU_VolumeShouldInject" bin/mesos-tests.sh` > > > Thanks, > > Kevin Klues > >
