You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2016/02/04 19:39:41 UTC

Review Request 43198: Added common appc spec utilities.

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

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 d4fd8cbfd5668da9ce4ac86baaadf42945c5abbd 
  src/appc/spec.cpp 324cdfec3766da4a8e324378a6e413477fa2b5d9 

Diff: https://reviews.apache.org/r/43198/diff/


Testing
-------

make check.


Thanks,

Jojy Varghese

Re: Review Request 43198: Added common appc spec utilities.

Posted by Jie Yu <yu...@gmail.com>.

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

Re: Review Request 43198: Added common appc spec utilities.

Posted by Jie Yu <yu...@gmail.com>.

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


Fix it, then Ship it!





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

    s/imageDir/imagePath/
    
    to be consistent.



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

    can you reorder the functions in cpp file to match the order in the hpp file?



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

    s/imageDir/imagePath/



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

    Please validate manifest as well.



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

    Hum, you want to do:
    ```
    validate = validateImageID(...);
    if (imageId.isSome()) {
      return Error("Invalid image ID: " + validate->message);
    }
    
    return None();
    ```


- Jie Yu


On Feb. 10, 2016, 3:11 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43198/
> -----------------------------------------------------------
> 
> (Updated Feb. 10, 2016, 3:11 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
> 
>