----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50523/#review146261 -----------------------------------------------------------
Some early comments. src/slave/containerizer/docker.cpp (line 949) <https://reviews.apache.org/r/50523/#comment212617> If this returns `Future`, then you may want to check `onFailed` as above. src/slave/containerizer/docker.cpp (line 1012) <https://reviews.apache.org/r/50523/#comment212615> The return value should be `Future<Nothing>` src/slave/containerizer/docker.cpp (line 1013) <https://reviews.apache.org/r/50523/#comment212614> s/std::string/string src/slave/containerizer/docker.cpp (lines 1016 - 1060) <https://reviews.apache.org/r/50523/#comment212616> two spaces src/slave/containerizer/docker.cpp (line 1018) <https://reviews.apache.org/r/50523/#comment212618> two spaces with above ``` Future<Nothing> inspect = docker->inspect(containerName, slave::DOCKER_INSPECT_DELAY) .then(defer(self(), [=](const Docker::Container& container) { ``` src/slave/containerizer/docker.cpp (line 1021) <https://reviews.apache.org/r/50523/#comment212620> I'd prefer ``` if (!deviceInspect.isSome()) { return Nothing(); } other logic here... ``` src/slave/containerizer/docker.cpp (line 1024) <https://reviews.apache.org/r/50523/#comment212619> s/std::string/const string src/slave/containerizer/docker.cpp (lines 1027 - 1029) <https://reviews.apache.org/r/50523/#comment212622> s/std::string/string src/slave/containerizer/docker.cpp (lines 1030 - 1031) <https://reviews.apache.org/r/50523/#comment212623> four spaces with above src/slave/containerizer/docker.cpp (lines 1031 - 1032) <https://reviews.apache.org/r/50523/#comment212624> new line here src/slave/containerizer/docker.cpp (line 1047) <https://reviews.apache.org/r/50523/#comment212625> s/.get()./-> src/slave/containerizer/docker.cpp (line 1055) <https://reviews.apache.org/r/50523/#comment212626> We should use a `continuation` here, you can refer to https://reviews.apache.org/r/50841/diff/5#index_header to get some hints. src/slave/containerizer/docker.cpp (line 1063) <https://reviews.apache.org/r/50523/#comment212630> s/std::set/set s/std::string/string src/slave/containerizer/docker.cpp (line 1064) <https://reviews.apache.org/r/50523/#comment212627> four spaces and s/std::string/string src/slave/containerizer/docker.cpp (line 1065) <https://reviews.apache.org/r/50523/#comment212628> kill this src/slave/containerizer/docker.cpp (lines 1065 - 1099) <https://reviews.apache.org/r/50523/#comment212629> two spaces is enough src/slave/containerizer/docker.cpp (line 1079) <https://reviews.apache.org/r/50523/#comment212631> s/Option<set < string>>/Option<set<string>> src/slave/containerizer/docker.cpp (lines 1081 - 1082) <https://reviews.apache.org/r/50523/#comment212633> new line here src/slave/containerizer/docker.cpp (lines 1081 - 1082) <https://reviews.apache.org/r/50523/#comment212635> new line src/slave/containerizer/docker.cpp (line 1082) <https://reviews.apache.org/r/50523/#comment212634> I'd prefer ``` if (!deviceJson.isSome()) { return None(); } other logic... ``` src/slave/containerizer/docker.cpp (line 1084) <https://reviews.apache.org/r/50523/#comment212636> s/std::vector/vector s/.get()./-> src/slave/containerizer/docker.cpp (line 1085) <https://reviews.apache.org/r/50523/#comment212637> I'd prefer ``` if (values.size() == 0) { return None(); } ``` src/slave/containerizer/docker.cpp (lines 1088 - 1089) <https://reviews.apache.org/r/50523/#comment212638> ``` Result<JSON::String> devicePath = value.as<JSON::Object>().find<JSON::String>("PathOnHost"); ``` src/slave/containerizer/docker.cpp (lines 1089 - 1090) <https://reviews.apache.org/r/50523/#comment212639> new line src/slave/containerizer/docker.cpp (line 1092) <https://reviews.apache.org/r/50523/#comment212632> s/.get()./-> src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 943) <https://reviews.apache.org/r/50523/#comment212640> This should be put into `#ifdef __linux__` src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 970) <https://reviews.apache.org/r/50523/#comment212641> s/.get()./-> src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 990 - 991) <https://reviews.apache.org/r/50523/#comment212642> new line src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 1006) <https://reviews.apache.org/r/50523/#comment212643> kill this src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 1060 - 1061) <https://reviews.apache.org/r/50523/#comment212644> s/.get()./-> src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 1063) <https://reviews.apache.org/r/50523/#comment212645> kill this - Guangya Liu On 七月 29, 2016, 6:15 p.m., Rajat Phull wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50523/ > ----------------------------------------------------------- > > (Updated 七月 29, 2016, 6:15 p.m.) > > > Review request for mesos, Benjamin Mahler, Kevin Klues, and Yubo Li. > > > Bugs: MESOS-5795 > https://issues.apache.org/jira/browse/MESOS-5795 > > > Repository: mesos > > > Description > ------- > > Updated docker recovery to account for GPU resources. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp 43ca4317d608b3b43dd7bd0d1b55c721e7364885 > src/slave/containerizer/docker.cpp 12bad2db03bcf755317c654f028b628c5c407a62 > src/tests/containerizer/nvidia_gpu_isolator_tests.cpp > 02d82e3e1ab3e7dd05f351a294774cf000276bcd > > Diff: https://reviews.apache.org/r/50523/diff/ > > > Testing > ------- > > GTEST_FILTER="NvidiaGpuDockerContainerizerTest.ROOT_DOCKER_LaunchWithGpuRecovery" > make -j check > > > Thanks, > > Rajat Phull > >
