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


Fix it, then Ship it!





src/slave/containerizer/fetcher.cpp (line 732)
<https://reviews.apache.org/r/52058/#comment221695>

    We are constructing the path at least 3 times. 
    
    How about 
    
    ```
    const string stdoutPath = path::join(info.sandbox_directory(), "stdout");
    ```
    
    and use the variable below?
    
    Same for stderr.



src/slave/containerizer/fetcher.cpp (line 740)
<https://reviews.apache.org/r/52058/#comment221696>

    Same deal. Use `stderrPath`.



src/slave/containerizer/fetcher.cpp (line 754)
<https://reviews.apache.org/r/52058/#comment221692>

    Below the line we should add a TODO
    
    ```
    TODO(megha.sharma): Fetcher should not create seperate stdout/stderr files 
but rather use FDs prepared by the container logger. See MESOS-6271 for more 
details.
    ```



src/slave/containerizer/fetcher.cpp (line 764)
<https://reviews.apache.org/r/52058/#comment221674>

    Missing space before `+`.



src/slave/containerizer/fetcher.cpp (line 766)
<https://reviews.apache.org/r/52058/#comment221687>

    s/ with error//
    
    The ":" is enough to indicate the error message that follows.



src/slave/containerizer/fetcher.cpp (line 781)
<https://reviews.apache.org/r/52058/#comment221688>

    s/ with error//
    
    The ":" is enough to indicate the error message that follows.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 591)
<https://reviews.apache.org/r/52058/#comment221675>

    "This test verified that" or "Tests that".



src/tests/containerizer/mesos_containerizer_tests.cpp (line 610)
<https://reviews.apache.org/r/52058/#comment221683>

    "exit 0" may be a better (more clear w.r.t intention) dummy executor 
command.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 626)
<https://reviews.apache.org/r/52058/#comment221678>

    `os::getuid(user)` didn't work?



src/tests/containerizer/mesos_containerizer_tests.cpp (line 631)
<https://reviews.apache.org/r/52058/#comment221679>

    s/std::string/string/
    
    s/stdOutPath/stdoutPath/
    
    stdout is common enough that we should just treat it as one word.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 636)
<https://reviews.apache.org/r/52058/#comment221680>

    s/std::string/string/
    
    s/stdErrPath/stderrPath/



src/tests/containerizer/mesos_containerizer_tests.cpp (line 642)
<https://reviews.apache.org/r/52058/#comment221676>

    2 space indentation.


- Jiang Yan Xu


On Oct. 13, 2016, 7:33 a.m., Megha Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2016, 7:33 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
>     https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> -------
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Reply via email to