Re: Review Request 24464: Redirect docker logs

[Jie Yu]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24464/#review50333
-----------------------------------------------------------

Ship it!


Looks good! Thanks for the cleanup!


src/docker/docker.hpp
<https://reviews.apache.org/r/24464/#comment88008>

    s/the/The/



src/docker/docker.hpp
<https://reviews.apache.org/r/24464/#comment88007>

    This fits in one line?



src/docker/docker.cpp
<https://reviews.apache.org/r/24464/#comment88009>

    Hum, why revert this change. No need to save the temp var 'msg'.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24464/#comment88010>

    s/containerName/container/



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24464/#comment88021>

    I would probably do the following (reads better):
    
    if (!containerLogs.contains(containerId)) {
      LOG(WARNING) << "...";
    
      ___destroy(containerId, killed, status);
      return;
    }
    
    Docker::Logs logs = containerLogs[containerId];
    
    logs.close();
    
    logs.wait().onAny(defer(
        self(),
        &____destroy,
        containerId,
        killed,
        status,
        lambda::_1));



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24464/#comment88031>

    One small issue here is that: if the 'docker logs' fails, we won't be able 
to notice that (through mesos logging) until the container is about to be 
destroyed. Worth adding a comment here.



src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24464/#comment88029>

    s/logStatus/waited/


- Jie Yu


On Aug. 12, 2014, 7:44 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24464/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2014, 7:44 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
> 
>