Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100387 --- Patch looks great! Reviews applied: [38137] All tests passed. -

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 5:34 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner/docker/puller.hpp, line 60 > > > > > > I think timeout should be part of the interface. remote puller > > especially

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 5:34 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner/docker/store.cpp, line 257 > > > > > > Dont we have to cleanup the created directory? It's automatically cleaned up in

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 24, 2015, 9:21 a.m.) Review request for mesos, Jie Yu, Jojy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 24, 2015, 8:02 p.m.) Review request for mesos, Jie Yu, Jojy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100452 --- Ship it! LGTM! Let's get this committed!

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100451 --- Ship it! Ship It! - Jojy Varghese On Sept. 24, 2015, 9:21

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100449 --- Ship it! Thanks for being patient. Please commit this!

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 24, 2015, 9 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioner/docker/local_puller.hpp, line 40 > > > > > > s/docker_discovery_local_dir/docker_local_archives_dir/ > > > > I

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 12:50 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioner/docker/puller.hpp, line 60 > > > > > > If we use LinkedHashMap we don't need to create another struct right? > > Timothy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100492 --- Patch looks great! Reviews applied: [38137] All tests passed. -

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioner/docker/local_puller.hpp, line 45 > > > > > > Not a strong opinion but if you think this could potentially be "not > >

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100226 --- src/slave/containerizer/provisioner/docker/puller.hpp (line 40)

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioners/docker.cpp, lines 104-107 > > > > > > I guess you are modeling this after the `docker pull` syntax here: > >

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100165 --- src/slave/containerizer/provisioner/docker/puller.hpp (line 60)

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review99919 --- Much closer! Haven't looked at the tests yet but will do.

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100090 --- src/slave/containerizer/provisioner/docker/local_puller.hpp (line

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 10, 2015, 3:04 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioners/docker.cpp, lines 104-107 > > > > > > I guess you are modeling this after the `docker pull` syntax here: > >

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100285 --- src/slave/containerizer/provisioner/docker/store.cpp (lines 210 -

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioner/docker/metadata_manager.hpp, line 57 > > > > > > With the pullers refactored out of the store I don't see the need for a >

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioner/docker/store.cpp, line 191 > > > > > > How large could this folder potentially be? To remove large directories > > I

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 5:34 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner/docker/puller.hpp, line 40 > > > > > > Shouldnt this be LayerInfo? I don't think we need to call it Info as it only holds

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 23, 2015, 7:50 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioner/docker/puller.hpp, line 60 > > > > > > If we use LinkedHashMap we don't need to create another struct right? True, but I

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote: > > src/Makefile.am, line 491 > > > > > > Similar to master/resgistry.proto, we should put this proto file under > > > >

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote: > > src/slave/containerizer/provisioner/docker/paths.hpp, line 40 > > > > > > What's this? No longer needed with the shared provisioner change, thanks! > On Sept. 21,

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote: > > src/slave/containerizer/provisioner/docker/store.cpp, line 256 > > > > > > According to your current design, Store is supposed to be agnostic to > > which puller

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote: > > src/slave/containerizer/provisioner/docker/store.cpp, line 256 > > > > > > According to your current design, Store is supposed to be agnostic to > > which puller

Re: Review Request 38137: Added Docker provisioner, store and local puller

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote: > > src/slave/containerizer/provisioner/docker/message.proto, line 21 > > > > > > Remove .docker? > > Timothy Chen wrote: > I added the docker namespace since it's

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100118 --- Patch looks great! Reviews applied: [38137] All tests passed. -

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 22, 2015, 10:15 p.m.) Review request for mesos, Jie Yu, Jojy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 22, 2015, 8:22 a.m.) Review request for mesos, Jie Yu, Jojy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review99772 --- Overall, looks good. The main issue is that 'Store' should not have

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 21, 2015, 5:06 p.m.) Review request for mesos, Jie Yu, Jojy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 21, 2015, 5:33 p.m.) Review request for mesos, Jie Yu, Jojy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 21, 2015, 5:50 p.m.) Review request for mesos, Jie Yu, Jojy

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review99775 --- Bad patch! Reviews applied: [38137] Failed command: make -j3

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review99768 --- Bad patch! Reviews applied: [38137] Failed command: make -j3

Re: Review Request 38137: Added Docker provisioner, store and local puller

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/ --- (Updated Sept. 21, 2015, 5:03 p.m.) Review request for mesos, Jie Yu, Jojy