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




src/launcher/executor.cpp (line 86)
<https://reviews.apache.org/r/42539/#comment178046>

    Please use camel case for variable names. So please use `_taskCommmand` 
here.



src/launcher/executor.cpp (line 99)
<https://reviews.apache.org/r/42539/#comment178047>

    Ditto.



src/slave/containerizer/mesos/containerizer.hpp (lines 224 - 264)
<https://reviews.apache.org/r/42539/#comment178064>

    No need to show this table here. you can just move it to the cpp file. This 
is implementation detail.



src/slave/containerizer/mesos/containerizer.hpp (line 268)
<https://reviews.apache.org/r/42539/#comment178107>

    If command executor is used, the provisionInfo here should be the 
provisionInfo returned for .rootfs volume, right? Have you figured out this 
part?



src/slave/containerizer/mesos/containerizer.cpp (line 995)
<https://reviews.apache.org/r/42539/#comment178072>

    s/commandInfo/executorLaunchCommand/



src/slave/containerizer/mesos/containerizer.cpp (line 996)
<https://reviews.apache.org/r/42539/#comment178071>

    I would rename it to `getExecutorLaunchCommand`



src/slave/containerizer/mesos/containerizer.cpp (line 999)
<https://reviews.apache.org/r/42539/#comment178067>

    The error message could be more informative:
    ```
    Failed to determine the executor launch command...
    ```



src/slave/containerizer/mesos/containerizer.cpp (line 1084)
<https://reviews.apache.org/r/42539/#comment178095>

    You want to move the commments in the header here. The method level 
comments should explain what this function is doing and returns what.
    
    You can put the giant table in the method body.



src/slave/containerizer/mesos/containerizer.cpp (lines 1084 - 1100)
<https://reviews.apache.org/r/42539/#comment178105>

    I would suggest the following flow for this function:
    
    1. Get 'command' in Mesos. The command is executorInfo.command if 
taskInfo.isNone(). Otherwise, use taskInfo.command().
    
    2. Use the table to calculate the final command based on docker manifest.
    
    3. if taskInfo.isNone(), return the command obtained in step 2. Otherwise, 
set override command flag for executorInfo.command().



src/slave/containerizer/mesos/containerizer.cpp (line 1100)
<https://reviews.apache.org/r/42539/#comment178100>

    s/commandInfo/command/



src/slave/containerizer/mesos/containerizer.cpp (line 1117)
<https://reviews.apache.org/r/42539/#comment178101>

    why not container_config here?



src/slave/containerizer/mesos/containerizer.cpp (line 1159)
<https://reviews.apache.org/r/42539/#comment178102>

    return Error?


- Jie Yu


On Jan. 29, 2016, 10:15 p.m., Gilbert Song wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42539/
> -----------------------------------------------------------
> 
> (Updated Jan. 29, 2016, 10:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-4004
>     https://issues.apache.org/jira/browse/MESOS-4004
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Support image specified Entrypoint and Cmd.
> 
> 
> Diffs
> -----
> 
>   src/launcher/executor.cpp 356d311fdf97b2c4663c60e13ede7cdb71a264c7 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 811ab7937279c4a55da450c136f9fcb1303ea0d5 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
> 
> Diff: https://reviews.apache.org/r/42539/diff/
> 
> 
> Testing
> -------
> 
> make check(ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Reply via email to