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




include/mesos/appc/spec.hpp (line 27)
<https://reviews.apache.org/r/43198/#comment179711>

    add a blank line below



include/mesos/appc/spec.hpp (line 46)
<https://reviews.apache.org/r/43198/#comment179693>

    Why Future here. I think Try should be sufficient. Let's use os::read 
instead of io::read.
    
    Also, the parameter is a 'Path' while the function right above this 
function is using 'string' as the image path. I think we should be consistent 
here, either using string or Path for all of them.



include/mesos/appc/spec.hpp (lines 69 - 71)
<https://reviews.apache.org/r/43198/#comment179709>

    Hum, the semantics of this validation function is complicated. Can we 
instead of a simpler validation function just for imagePath?
    ```
    Option<Error> validate(const std::string& imagePath)
    {
      // validate layout
      // validate manifest
      // validate image id
    }
    ```
    
    We can pull the name/label matching to the top level (does not belong to 
validate, maybe introduce a 'match' function in appc::Store).



src/appc/spec.cpp (line 128)
<https://reviews.apache.org/r/43198/#comment179708>

    Please use os::read instead.



src/appc/spec.cpp (lines 160 - 185)
<https://reviews.apache.org/r/43198/#comment179705>

    This label comparision is nasty. I think one of the reason is because we're 
using mesos::Labels in Image::Appc while appc::spec::ImageManifest is using its 
own Label.
    
    Can you first change Image::Appc:
    ```
    message Appc {
      required string name = 1;
      optional string id = 2;
      repeated appc::spec::Label labels = 3;
    }
    ```
    (You need to pull Label in ImageManifest to the top level).
    
    Then, add a C++ appc::spec::Labels class to wrap the repeated protobuf 
field (like we did for Resource) in spec.hpp|cpp
    
    Then, you can introduce a == operator for Labels in spec.hpp|cpp.



src/appc/spec.cpp (line 192)
<https://reviews.apache.org/r/43198/#comment179710>

    Let's move this function to appc::Store. Simple discovery is already 
removed from the spec, we're just add this as one of our extention to the spec.



src/appc/spec.cpp (line 214)
<https://reviews.apache.org/r/43198/#comment179698>

    I got confused, we do we need to call os::uname for this? The target image 
should have nothing to do with the os/arch of the host, right (even default 
value does not make sense to me).


- Jie Yu


On Feb. 9, 2016, 7:40 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43198/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2016, 7:40 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
>     https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change adds common utility functions such as :
>   - validating image information against actual data in the image directory.
>   - getting list of dependencies at depth 1 for an image.
>   - getting image path simple image discovery.
> 
> 
> Diffs
> -----
> 
>   include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
>   src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 
> 
> Diff: https://reviews.apache.org/r/43198/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to