Re: Review Request 34137: Add support for container image provisioners.

2015-07-21 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92462 --- Ship it! Ship It! - Jie Yu On July 12, 2015, 4:47 a.m., Ian

Re: Review Request 34137: Add support for container image provisioners.

2015-07-21 Thread Jie Yu
On July 17, 2015, 1:36 a.m., Jie Yu wrote: src/slave/containerizer/mesos/containerizer.cpp, lines 637-643 https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line637 No test for checkpointing??? I decided to punt this in this review. Will follow up with a test

Re: Review Request 34137: Add support for container image provisioners.

2015-07-20 Thread Jie Yu
On July 17, 2015, 1:36 a.m., Jie Yu wrote: src/slave/containerizer/mesos/containerizer.cpp, line 630 https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line630 Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is

Re: Review Request 34137: Add support for container image provisioners.

2015-07-20 Thread Timothy Chen
On July 17, 2015, 1:36 a.m., Jie Yu wrote: src/slave/containerizer/provisioner.cpp, line 24 https://reviews.apache.org/r/34137/diff/3/?file=1009145#file1009145line24 Where is this? This won't compile! You're right it won't, wierd it compiled for me :( let's fix this! - Timothy

Re: Review Request 34137: Add support for container image provisioners.

2015-07-20 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92339 --- src/slave/containerizer/mesos/containerizer.cpp (lines 185 - 186)

Re: Review Request 34137: Add support for container image provisioners.

2015-07-17 Thread Jiang Yan Xu
On July 16, 2015, 6:36 p.m., Jie Yu wrote: src/slave/containerizer/mesos/containerizer.cpp, line 630 https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line630 Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is

Re: Review Request 34137: Add support for container image provisioners.

2015-07-16 Thread Timothy Chen
On July 8, 2015, 11:34 p.m., Vinod Kone wrote: src/slave/containerizer/mesos/containerizer.cpp, line 418 https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line418 Is it possible for rootfs to not exist when we are here? If not, there should be a CHECK and a comment

Re: Review Request 34137: Add support for container image provisioners.

2015-07-16 Thread Timothy Chen
On July 14, 2015, 7:41 p.m., Vinod Kone wrote: src/slave/containerizer/provisioner.cpp, lines 43-56 https://reviews.apache.org/r/34137/diff/2-3/?file=989758#file989758line43 Why do you need foreach loop here if you were going to return error anyway? Timothy Chen wrote: We

Re: Review Request 34137: Add support for container image provisioners.

2015-07-16 Thread Timothy Chen
On May 28, 2015, 9:49 p.m., Paul Brett wrote: src/slave/containerizer/mesos/containerizer.hpp, line 245 https://reviews.apache.org/r/34137/diff/1/?file=957263#file957263line245 To many underscores - can we come up with a better name? We can refactor later. - Timothy

Re: Review Request 34137: Add support for container image provisioners.

2015-07-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review92000 --- Tim, I probably wound wait for Vinod's shipit before committing

Re: Review Request 34137: Add support for container image provisioners.

2015-07-15 Thread Timothy Chen
On July 14, 2015, 7:41 p.m., Vinod Kone wrote: src/slave/containerizer/provisioner.cpp, lines 43-56 https://reviews.apache.org/r/34137/diff/2-3/?file=989758#file989758line43 Why do you need foreach loop here if you were going to return error anyway? We need the foreach to go

Re: Review Request 34137: Add support for container image provisioners.

2015-07-15 Thread Timothy Chen
On July 14, 2015, 7:41 p.m., Vinod Kone wrote: can you fix the depends on please? I think the two listed are the correct dependencies for this rb? - Timothy --- This is an automatically generated e-mail. To reply, visit:

Re: Review Request 34137: Add support for container image provisioners.

2015-07-14 Thread Vinod Kone
On July 8, 2015, 11:34 p.m., Vinod Kone wrote: can you please set the dependency for this review correctly? it's hard to understand which reviews introduced the code that this review depends on. none of the dropped issues have comments. did you forget to hit publish on your comments? -

Re: Review Request 34137: Add support for container image provisioners.

2015-07-14 Thread Vinod Kone
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review91662 --- can you fix the depends on please?

Re: Review Request 34137: Add support for container image provisioners.

2015-07-11 Thread Ian Downes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 11, 2015, 9:47 p.m.) Review request for mesos, Chi Zhang, Paul

Re: Review Request 34137: Add support for container image provisioners.

2015-07-10 Thread Ian Downes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 10, 2015, 5:05 p.m.) Review request for mesos, Chi Zhang, Paul

Re: Review Request 34137: Add support for container image provisioners.

2015-07-08 Thread Vinod Kone
On June 29, 2015, 7:41 p.m., Jiang Yan Xu wrote: src/slave/containerizer/provisioner.cpp, lines 46-47 https://reviews.apache.org/r/34137/diff/2/?file=989758#file989758line46 Seems like this belongs to a later patch. AppcProvisioner is not introduced yet. +1 - Vinod

Re: Review Request 34137: Add support for container image provisioners.

2015-07-07 Thread Ian Downes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul

Re: Review Request 34137: Add support for container image provisioners.

2015-06-30 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review89975 --- src/slave/containerizer/mesos/containerizer.cpp (line 626)

Re: Review Request 34137: Add support for container image provisioners.

2015-06-29 Thread Jiang Yan Xu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review89293 --- src/slave/containerizer/mesos/containerizer.hpp (lines 319 - 320)

Re: Review Request 34137: Add support for container image provisioners.

2015-06-24 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review89227 --- include/mesos/slave/isolator.hpp (line 71)

Re: Review Request 34137: Add support for container image provisioners.

2015-06-22 Thread Ian Downes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated June 22, 2015, 9:44 a.m.) Review request for mesos, Chi Zhang, Paul

Re: Review Request 34137: Add support for container image provisioners.

2015-05-22 Thread Ian Downes
On May 14, 2015, 12:41 p.m., Timothy Chen wrote: src/slave/containerizer/mesos/containerizer.cpp, line 193 https://reviews.apache.org/r/34137/diff/1/?file=957264#file957264line193 Not sure if I follow this logic or if I'm missing something, but the provisioners hashmap seems to

Review Request 34137: Add support for container image provisioners.

2015-05-12 Thread Ian Downes
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.