Re: Review Request 34139: AppC image discovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/#review141467 --- Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews. - Joris Van Remoortere 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.
--- 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/#comment13 +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.
--- 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.
--- 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.
--- 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.
--- 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
Re: Review Request 34139: AppC image discovery.
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.
--- 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.
--- 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
