You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/05/13 02:48:00 UTC
Review Request 34139: AppC image discovery.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34139/
-----------------------------------------------------------
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 14bc976a7b6a656fb58085484d25c3de3cf0f693
src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION
src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION
src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
Diff: https://reviews.apache.org/r/34139/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34139: AppC image discovery.
Posted by Ian Downes <ia...@gmail.com>.
> On May 14, 2015, 2:05 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc/discovery.cpp, line 34
> > <https://reviews.apache.org/r/34139/diff/1/?file=957272#file957272line34>
> >
> > static?
This function is only going to be called once anyway though.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34139/#review83825
-----------------------------------------------------------
On May 12, 2015, 5:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34139/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:47 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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34139: AppC image discovery.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34139/#review83825
-----------------------------------------------------------
src/slave/containerizer/provisioners/appc/discovery.hpp
<https://reviews.apache.org/r/34139/#comment134902>
How about namespacing this under appc as well? mesos::internal::slave::Discovery seems pretty generic to me.
src/slave/containerizer/provisioners/appc/discovery.cpp
<https://reviews.apache.org/r/34139/#comment134903>
static?
src/slave/containerizer/provisioners/appc/discovery.cpp
<https://reviews.apache.org/r/34139/#comment134904>
Can you print the unsupported strategy?
src/slave/flags.cpp
<https://reviews.apache.org/r/34139/#comment134900>
Do you think we should namespace these flags with appc? Seems like appc so far is the only one that has a configurable discvoery mechanism and local dir
- Timothy Chen
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34139/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34139: AppC image discovery.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34139/#review84196
-----------------------------------------------------------
src/slave/containerizer/provisioners/appc/discovery.hpp
<https://reviews.apache.org/r/34139/#comment135337>
AppcImage is introduced in r/34142
src/slave/containerizer/provisioners/appc/discovery.cpp
<https://reviews.apache.org/r/34139/#comment135389>
Please see my comment in r/34142. I would like to suggest have AppcImage drive discover instead of AppcPP.
src/slave/containerizer/provisioners/appc/discovery.cpp
<https://reviews.apache.org/r/34139/#comment135390>
nit: this call is duplicated? maybe have discover just understand a raw canonicalized string?
src/slave/flags.cpp
<https://reviews.apache.org/r/34139/#comment135335>
should introduce all avaiable options to users here? Ditto to other flags introduced?
- Chi Zhang
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34139/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34139: AppC image discovery.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34139/#review90955
-----------------------------------------------------------
src/slave/flags.cpp (line 69)
<https://reviews.apache.org/r/34139/#comment144112>
We should make the default /tmp/mesos/images/appc
- Timothy Chen
On July 7, 2015, 7: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, 7: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
>
>
Re: Review Request 34139: AppC image discovery.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>
Re: Review Request 34139: AppC image discovery.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34139/#review91154
-----------------------------------------------------------
tests!?
src/slave/containerizer/provisioners/appc/discovery.hpp (line 36)
<https://reviews.apache.org/r/34139/#comment144435>
This class and methods all need commenting!
src/slave/containerizer/provisioners/appc/discovery.hpp (line 50)
<https://reviews.apache.org/r/34139/#comment144437>
comments.
src/slave/containerizer/provisioners/appc/discovery.hpp (line 68)
<https://reviews.apache.org/r/34139/#comment144438>
comments.
src/slave/containerizer/provisioners/appc/discovery.cpp (line 60)
<https://reviews.apache.org/r/34139/#comment144443>
+1
please make sure that code in a review only depends on stuff in the current review and its dependent reviews. it's hard to review o.w.
- Vinod Kone
On July 7, 2015, 7: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, 7: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
>
>
Re: Review Request 34139: AppC image discovery.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Updated dependencies.
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
Re: Review Request 34139: AppC image discovery.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34139/
-----------------------------------------------------------
(Updated June 22, 2015, 9:54 a.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Flag name changes. Switched to discovery returning at most a single URI.
Repository: mesos
Description
-------
Local and simple discovery only.
Diffs (updated)
-----
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