mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <...@jxu.me>
Subject Re: Review Request 34139: AppC image discovery.
Date Wed, 08 Jul 2015 00:28:03 GMT

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



src/slave/containerizer/provisioners/appc/discovery.hpp (lines 43 - 46)
<https://reviews.apache.org/r/34139/#comment143985>

    Some comments explaining that the return value is a URI string would be nice.



src/slave/containerizer/provisioners/appc/discovery.hpp (line 46)
<https://reviews.apache.org/r/34139/#comment143994>

    The `id` is not use in this review and not specified in https://github.com/appc/spec/blob/master/spec/discovery.md
    I assume its the sha512 but is it used during discovery?



src/slave/containerizer/provisioners/appc/discovery.hpp (line 79)
<https://reviews.apache.org/r/34139/#comment144003>

    No need for the trailing `;`



src/slave/containerizer/provisioners/appc/discovery.cpp (line 21)
<https://reviews.apache.org/r/34139/#comment143999>

    Kill empty line.



src/slave/containerizer/provisioners/appc/discovery.cpp (line 60)
<https://reviews.apache.org/r/34139/#comment143977>

    canonicalize() not introduced until /r/34142/.
    Is there anything else outside discovery that uses the method? Can it be moved to class
`Discovery` (so we don't have to deal with cyclic review dependency)?



src/slave/containerizer/provisioners/appc/discovery.cpp (line 90)
<https://reviews.apache.org/r/34139/#comment143998>

    FWIW https://github.com/appc/spec/blob/master/spec/discovery.md uses https exclusively.



src/slave/flags.cpp (line 63)
<https://reviews.apache.org/r/34139/#comment144000>

    List all supported values?



src/slave/flags.cpp (line 68)
<https://reviews.apache.org/r/34139/#comment144001>

    State that this is only for local discovery?
    
    The sentences already mentions 'local images' but I think --appc_discovery=local is more
explict in telling what the operator should do.


- Jiang Yan Xu


On July 7, 2015, 12:42 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34139/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Local and simple discovery only.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34139/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>


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