mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gyliu...@gmail.com>
Subject Re: Review Request 49348: Added implementation to Appc Runtime Isolator.
Date Sat, 02 Jul 2016 10:19:48 GMT

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




src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 80)
<https://reviews.apache.org/r/49348/#comment205855>

    As your previous patches are using `Appc` in comments, can you please use `Appc` here
and other comments as well?



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 87 - 88)
<https://reviews.apache.org/r/49348/#comment205856>

    One line



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 161)
<https://reviews.apache.org/r/49348/#comment205878>

    Can you please add a comment here just like what we did for docker run time isolator for
how to handle duplicate env vars?
    
    // Keep all environment from runtime isolator. If there exists
    // environment variable duplicate cases, it will be overwritten
    // in mesos containerizer.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 162)
<https://reviews.apache.org/r/49348/#comment205877>

    kill this line



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 171)
<https://reviews.apache.org/r/49348/#comment205879>

    s/form/from



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 192)
<https://reviews.apache.org/r/49348/#comment205880>

    kill this line



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 202 - 205)
<https://reviews.apache.org/r/49348/#comment205882>

    1. s/Exec/'Exec'
    2. Add period to the end of each comment.
    
    It would be great to add more comments to the end for the commnets by refering to the
row of the table.
    
    Such as:
    // 1. If 'shell' is false,  and 'value' is not set, use Exec from the image. (row 1-2)
    // 2. If 'shell' is false and 'value' is set, ignore Exec from the image. (row 3-4)
    // 3. If 'shell' is true and 'value' is not set, return error. (row 5-6)
    // 4. If 'shell' is true and 'value' is set, ignore Exec from the image. (row 7-8)



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 243)
<https://reviews.apache.org/r/49348/#comment205883>

    I think checking `!command.has_value()` is good enough. The logic here has some problem,
if the comand does not have value then the check of `comand.value()` may core dump here.
    
    if (!command.has_value()) {
      return Error("Shell specified but no command value provided");
    }
    
    Or do you mean `if (!command.has_value() || !command.value().empty()) {` here.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 257 - 258)
<https://reviews.apache.org/r/49348/#comment205884>

    new line here



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 282)
<https://reviews.apache.org/r/49348/#comment205881>

    s/WorkingDir/workingDirectory


- Guangya Liu


On 七月 1, 2016, 9:44 p.m., Srinivas Brahmaroutu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49348/
> -----------------------------------------------------------
> 
> (Updated 七月 1, 2016, 9:44 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation to Appc Runtime Isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49348/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>


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