mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 42539: Support image specified Entrypoint and Cmd.
Date Fri, 29 Jan 2016 23:15:32 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message