mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilbert Song <songzihao1...@gmail.com>
Subject Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.
Date Tue, 21 Jun 2016 17:41:57 GMT

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



Please address my comments and followings:

1. Could you add the JIRA ticket MESOS-4778 to `BUG`. That would create a link from review
board to JIRA ticket automatically.
2. Could you mention what test you have done on `test done` section?
3. Please make sure to mannually test with a real appc image, and verify the default executable(s)
run correctly.
4. Could you move the test case to a separate patch (appreciated that would make review easier).


include/mesos/appc/spec.proto (lines 59 - 60)
<https://reviews.apache.org/r/46498/#comment204081>

    Please rephase this comment depending on the spec:
    
    https://github.com/appc/spec/blob/master/spec/aci.md#image-manifest-schema



include/mesos/appc/spec.proto (line 61)
<https://reviews.apache.org/r/46498/#comment204080>

    Fix the indentation.



include/mesos/appc/spec.proto (lines 62 - 64)
<https://reviews.apache.org/r/46498/#comment204082>

    Did you test your patch with a real appc image runtime?
    
    It seems to me the `Exec` will not be parsed properly. You may not capture all executable
strings.



include/mesos/appc/spec.proto (line 66)
<https://reviews.apache.org/r/46498/#comment204083>

    Please see other `TODO` for correct formatting. Thanks:)



include/mesos/appc/spec.proto (lines 66 - 75)
<https://reviews.apache.org/r/46498/#comment204086>

    Could we move this above annotation and dependency? Let's make it same order as it spec.
More clear for people to read it. :)



include/mesos/appc/spec.proto (line 70)
<https://reviews.apache.org/r/46498/#comment204084>

    Should this be repeated string?



include/mesos/appc/spec.proto (line 72)
<https://reviews.apache.org/r/46498/#comment204092>

    hmm..seems like we can reuse the `name`-`value` pair from the label. I would suggest introduce
another protobuf message `Environment`.



include/mesos/slave/isolator.proto (lines 87 - 92)
<https://reviews.apache.org/r/46498/#comment204093>

    Please move it below (at TODO position).



include/mesos/slave/isolator.proto (line 91)
<https://reviews.apache.org/r/46498/#comment204094>

    Let's use 10 for proto field #. #6 may be used before. This may make it backward incompatible.


- Gilbert Song


On June 2, 2016, 8:45 p.m., Srinivas Brahmaroutu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> -----------------------------------------------------------
> 
> (Updated June 2, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add runtime for Appc Spec ex: command, workingdir and environment.
> 
> 
> Diffs
> -----
> 
>   include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
>   include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
>   src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
>   src/slave/containerizer/mesos/containerizer.cpp c7b9744463cf8e1921dcb5e2b7dec7d4e2c0e45f

>   src/slave/containerizer/mesos/isolators/appc/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp aaa0efe63e587b9e604082b52a3cb8c11545fbb9

>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 48a05059969e068a0ee0d38b61be9e7104e3188d

>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 7540be6d8a412eb3d380d315c59223236d3eff67

>   src/slave/containerizer/mesos/provisioner/store.hpp 1d477ef13ddd24fd8badae0decaa4a2271ecc746

>   src/tests/containerizer/provisioner_appc_tests.cpp 84fe52b6937c3b7d7628b17a2f045eec2f386b4d

> 
> Diff: https://reviews.apache.org/r/46498/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>


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