----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24464/#review50206 -----------------------------------------------------------
Synced with BenH on this patch. Here is our comments. Let us know if you have any question! Thanks Tim! src/docker/docker.hpp <https://reviews.apache.org/r/24464/#comment87883> s/LogStream/Logs/ so you'll have logs.out() and logs.err() which reads pretty good. src/docker/docker.hpp <https://reviews.apache.org/r/24464/#comment87878> Instead of saving pid, out, err, status, could we just store the subprocess itself? That's much more clear to me. Also, I'm not convinced that we should save containerName in Logs class because you'll just need that for logging, right? These loggings are supposed to be done by the containerizer. src/docker/docker.hpp <https://reviews.apache.org/r/24464/#comment87905> Instead of overloading close() with some 'wait' semantics, what about having another function 'wait()': // Stop the log stream. (Please remove the const as a // close does not sound like a const function). void close() { if (s.status().isPending()) { :kill(...); } } // Block until the underlying log process is terminated. // Returns a failure if any error occurs. Future<Nothing> wait() { return s.status() .then(lambda::bind(&_wait, lambda::_1)); } src/docker/docker.hpp <https://reviews.apache.org/r/24464/#comment87885> s/containerName/container/ Here and everywhere else so that it's consistent with other functions (e.g. '_kill' below). src/docker/docker.cpp <https://reviews.apache.org/r/24464/#comment87914> These can be killed. See my comments above. src/docker/docker.cpp <https://reviews.apache.org/r/24464/#comment87911> See my comments above. You can kill those VLOGs. src/docker/docker.cpp <https://reviews.apache.org/r/24464/#comment87912> Ditto. src/docker/docker.cpp <https://reviews.apache.org/r/24464/#comment87918> return Logs(s); You probably want to make the constructor of Docker::Logs private and add Docker class to the friend list of Docker::Logs. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87924> s/logStreams/logs/ src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87925> We may want to rename this function to avoid name collision with member field 'logs'. s/logs/logging/ src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87926> s/containerName/name src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87929> Please take a look at src/slave/containerizer/mesos/containerizer.cpp where redirection is handled. You need to close 'out' and 'err' at least. You may also wanna do cloexec on out and err. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87820> Kill extra line here. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87927> logs.put(containerId, _logs); You may want to move this up before opening 'stdout/stderr'. This is because the opening may fail, if that's the case, you still want to cleanup the log process, right? src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87931> You probably want to do a 'logs.wait()' here: logs.wait() .onAny(defer(self(), &Self::__logging, ...); Inside __logging, you can add some logging. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/24464/#comment87887> CHECK(!status.isPending()); src/tests/docker_containerizer_tests.cpp <https://reviews.apache.org/r/24464/#comment87889> EXPECT_FALSE(strings::contains(..., "out!")); as well to make sure stdout output is not in stderr. - Jie Yu On Aug. 11, 2014, 8:19 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24464/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2014, 8:19 a.m.) > > > Review request for mesos, Benjamin Hindman and Jie Yu. > > > Repository: mesos-git > > > Description > ------- > > Review: https://reviews.apache.org/r/24464 > > > Diffs > ----- > > src/docker/docker.hpp 98b2d6099988f51f12e7b108e73dcfd0143adc48 > src/docker/docker.cpp 1cba381118c6bd2ac7fcf5a8a229602e2c65c571 > src/slave/containerizer/docker.cpp 904cdd32362591777aecaa58e723af36419f011c > src/tests/docker_containerizer_tests.cpp > a559836dd11a9a97e5939364c4b35a8dbb6a503d > > Diff: https://reviews.apache.org/r/24464/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
