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 43198: Added common appc spec utilities.
Date Tue, 09 Feb 2016 21:48:07 GMT

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


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