Re: Review Request 37197: Docker image store.

[Timothy Chen]

> On Aug. 31, 2015, 8:09 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 192
> > 
> >
> > I think path::join just returns a string

This is removed later on.


- Timothy


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

> On Sept. 3, 2015, 6:16 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 49
> > 
> >
> > Is this intended to be an abstract class? If so, why this factory 
> > method?

It's going to be used in a factory therefore we created this, as users can 
choose which store to use. And yes it's intended to be abstract.


> On Sept. 3, 2015, 6:16 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 51
> > 
> >
> > What is the ownership model for fetcher pointer. By ownership I mean 
> > how is the lifecycle of the pointer managed?

Good question, it's passed all the way from provisioner factory interface, and 
assumed to be owned external. 
We can leave a TODO on the provisioner provision interface to fix this later.


> On Sept. 3, 2015, 6:16 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 55
> > 
> >
> > I think all new code needs to follow mesos doxygen guidelines.

It is in the latest review.


> On Sept. 3, 2015, 6:16 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 93
> > 
> >
> > According to 
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_Order,
> >  the non-copyable/non-assignable section should be last in the declaration 
> > order.

It is in the latest review.


> On Sept. 3, 2015, 6:16 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 150
> > 
> >
> > calls for a static constant?

It is in the latest review.


- Timothy


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

> On Aug. 26, 2015, 5:50 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 135
> > 
> >
> > I am not a big fan of such magic numbers (referring to that '7'). How 
> > about following the pattern the fetcher is using here 
> > (src/slave/containerizer/fetcher.cpp)?
> > 
> > 
> > ```
> > [...]
> > namespace slave {
> > 
> > static const string FILE_URI_PREFIX = "file://";
> > [...]
> > 
> > if (strings::startsWith(imageUri, FILE_URI_PREFIX)) {
> > imageUri = imageUri.substr(FILE_URI_PREFIX.size());
> > }
> > ```
> 
> Till Toenshoff wrote:
> Why exactly did you close this without fixing the issue or giving some 
> more insights on why you insist on magic numbers?

Sorry I dropped it because this code is removed later on. I opened up a new 
review that holds everything now.


- Timothy


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

> On Sept. 3, 2015, 6:59 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 100
> > 
> >
> > Does this need to be in the hpp file?

It is in the latest review.


> On Sept. 3, 2015, 6:59 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 146
> > 
> >
> > I would rethink the lifecycle management of this poiner.

We should rethink to fix this from the provisioner interface.


> On Sept. 3, 2015, 6:59 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 49
> > 
> >
> > Ususally if you want to provide a factory class that hands out 
> > implementations, you use Factory class (commonly known as Factory design 
> > pattern).

In this case Store is providing this factory as we have multple implemetations 
later on.


- Timothy


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Till Toenshoff]

> On Aug. 26, 2015, 5:50 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 135
> > 
> >
> > I am not a big fan of such magic numbers (referring to that '7'). How 
> > about following the pattern the fetcher is using here 
> > (src/slave/containerizer/fetcher.cpp)?
> > 
> > 
> > ```
> > [...]
> > namespace slave {
> > 
> > static const string FILE_URI_PREFIX = "file://";
> > [...]
> > 
> > if (strings::startsWith(imageUri, FILE_URI_PREFIX)) {
> > imageUri = imageUri.substr(FILE_URI_PREFIX.size());
> > }
> > ```

Why exactly did you close this without fixing the issue or giving some more 
insights on why you insist on magic numbers?


- Till


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.hpp (line 100)


Does this need to be in the hpp file?



src/slave/containerizer/provisioners/docker/store.hpp (line 146)


I would rethink the lifecycle management of this poiner.



src/slave/containerizer/provisioners/docker/store.cpp (line 49)


Ususally if you want to provide a factory class that hands out 
implementations, you use Factory class (commonly known as Factory design 
pattern).



src/slave/containerizer/provisioners/docker/store.cpp (line 139)


magic numbers (7) is not recommended.



src/slave/containerizer/provisioners/docker/store.cpp (line 146)


What is the workflow for creating these directories?



src/slave/containerizer/provisioners/docker/store.cpp (line 238)


const?


- Jojy Varghese


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.hpp (line 49)


Is this intended to be an abstract class? If so, why this factory method?



src/slave/containerizer/provisioners/docker/store.hpp (line 51)


What is the ownership model for fetcher pointer. By ownership I mean how is 
the lifecycle of the pointer managed?



src/slave/containerizer/provisioners/docker/store.hpp (line 55)


I think all new code needs to follow mesos doxygen guidelines.



src/slave/containerizer/provisioners/docker/store.hpp (line 93)


According to 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_Order,
 the non-copyable/non-assignable section should be last in the declaration 
order.



src/slave/containerizer/provisioners/docker/store.cpp (line 150)


calls for a static constant?



src/slave/flags.hpp (line 52)


Wondering we should stuff everything in Flags class or if we can use 
inheritance to make  feature specfic flags.


- Jojy Varghese


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

> On Aug. 31, 2015, 8:09 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 131
> > 
> >
> > For local store I don't think this comment is valid.
> > Should just comment that only local images with file:// is supported.

The implemenation gets changed dramatically in the commit 37200 (refactoring 
docker image struct) so are these comments here still relevant?


> On Aug. 31, 2015, 8:09 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 247
> > 
> >
> > If tar is terminated in the middle, will this operation succeed again? 
> > We only check the directory exist to signal that we untarred the layer, but 
> > it might not necessarily really finished.
> > I Think we will have to do a mv here as well

So untar into a different temp directory and rename it to appear in the staging 
directory?


- Lily


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/store.cpp (line 131)


For local store I don't think this comment is valid.
Should just comment that only local images with file:// is supported.



src/slave/containerizer/provisioners/docker/store.cpp (line 150)


does path::join return a Try? I was curious about this of what kind of 
error can it come back with, and I think it actually just returns a String.



src/slave/containerizer/provisioners/docker/store.cpp (line 192)


I think path::join just returns a string



src/slave/containerizer/provisioners/docker/store.cpp (line 247)


If tar is terminated in the middle, will this operation succeed again? We 
only check the directory exist to signal that we untarred the layer, but it 
might not necessarily really finished.
I Think we will have to do a mv here as well


- Timothy Chen


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

> On Aug. 26, 2015, 5:50 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 54
> > 
> >
> > `DockerImage` is not defined by this RR but by 37198. We usually go 
> > with one of those two options here;
> > A. mark the entire chain as "must get committed entirely"
> > B. make sure each RR is committable without breaking build (or function)

We'll commit all together.


- Timothy


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


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 27, 2015, 11:39 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
and Timothy Chen.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 27, 2015, 9:49 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
and Timothy Chen.


Changes
---

Addressed comments.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

> On Aug. 26, 2015, 5:50 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 126
> > 
> >
> > s/uri/uri schemes are/ ?

Most of the comments pertaining to uris is changed in review 37200 when the 
DockerImage struct is refactored.


> On Aug. 26, 2015, 5:50 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 211
> > 
> >
> > I thought we got rid of `os::basename` and suggest using 
> > `Path::basename` instead?!
> > `::basename` (which os::basename was relying on) is not thread-safe on 
> > some systems, please avoid it.

os::basename is no longer necessary when the DockerImage scheme changes in 
37200.


- Lily


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


On Aug. 26, 2015, 5:56 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 26, 2015, 5:56 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/store.cpp (lines 273 - 274)


Should we put this in the shared utils as well?
Seems like a very common pattern


- Timothy Chen


On Aug. 26, 2015, 5:56 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 26, 2015, 5:56 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Till Toenshoff]

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


Great work Lily, very clean considering that this is your first bigger 
contribution so far (hope I did not miss something here :) ). I really like the 
way things are structured, elegantly scaling.

I am puzzled on how you were able to get this compiled as you are using 
"os::basename" which got removed from stout a while ago and I am assuming that 
you dont bring it back later in this chain. I may have omitted vital 
explanations in some of the issues I raised - please feel free to ask for 
clarification / reasoning where needed.

Once you addressed these issues, I would like to give the chain another pass, 
hence I will add myself as a reviewer, hoping you are OK with that.


src/Makefile.am (line 745)


Formatting looks off in reviewboard -- please check if you are using 
hard-tabs (displayed as 8 chars).



src/slave/containerizer/provisioners/docker/store.hpp (line 29)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker/store.hpp (line 33)


Missing 
```
#include 
```



src/slave/containerizer/provisioners/docker/store.hpp (line 36)


Missing
```
#include "slave/containerizer/fetcher.hpp"
```



src/slave/containerizer/provisioners/docker/store.hpp (line 42)


How about a short descriptive comment on the purpose of this interface?



src/slave/containerizer/provisioners/docker/store.hpp (line 54)


`DockerImage` is not defined by this RR but by 37198. We usually go with 
one of those two options here;
A. mark the entire chain as "must get committed entirely"
B. make sure each RR is committable without breaking build (or function)



src/slave/containerizer/provisioners/docker/store.hpp (line 61)


Consider adding a protected default constructor to underline the fact that 
this is a pure virtual interface.



src/slave/containerizer/provisioners/docker/store.hpp (line 91)


Insert blank line here please.



src/slave/containerizer/provisioners/docker/store.cpp (line 28)


This should go above the stout headers.



src/slave/containerizer/provisioners/docker/store.cpp (line 32)


This should be the first include.



src/slave/containerizer/provisioners/docker/store.cpp (line 34)


We should avoid such general "using namespace xxx" as it pollutes our local 
namespace. Prefer explicit references as in "using process::Process" as also 
described in the google styleguide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

However, I did not raise an issue here because our codebase shows many uses 
of "using namespace process;", so maybe we actually want to trump the google 
styleguide in specific cases? I vaguely remember we had a discussion around 
this a while back but can not find the results right now -- maybe others can 
chime in here?



src/slave/containerizer/provisioners/docker/store.cpp (line 46)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/store.cpp (line 61)


Insert blank line please.



src/slave/containerizer/provisioners/docker/store.cpp (line 103)


Insert blank line please.



src/slave/containerizer/provisioners/docker/store.cpp (line 126)


s/uri/uri schemes are/ ?



src/slave/containerizer/provisioners/docker/store.cpp (line 133)


How about `imagePath` or even `path`? instead of `imageUri` as you are 
effectively trimming it from a URI to a local path?



src/slave/containerizer/provisioners/docker/store.cpp (line 135)


I am not a big fan of such magic numbers (referring to that '7'). How about 
following the pattern the fetcher is using here 
(src/slave/containerizer/fetcher.cpp)?

```
[...]
namespace slave {

static const string FILE_URI_PREFIX = "file://";
[...]

if (strings::startsWith(imageUri, FILE_URI_PREFIX)) {
imageUri = imageUri.substr(FILE_URI_PREFIX

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 25, 2015, 8:57 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Changes
---

Rebased on master.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 25, 2015, 6:47 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Changes
---

Rebased on master.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 25, 2015, 1:31 a.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Changes
---

Addressed comment on log message.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

> On Aug. 19, 2015, 6:11 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 1
> > 
> >
> > Btw this is actually renamed to be local store later right?
> > 
> > How about just remove the impl completely in this review so later 
> > review we introduce the local store?

Squashed commit that moves store implementation to LocalStore into this commit.


- Lily


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


On Aug. 19, 2015, 6:41 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 19, 2015, 6:41 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 19, 2015, 6:41 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/store.cpp (line 1)


Btw this is actually renamed to be local store later right?

How about just remove the impl completely in this review so later review we 
introduce the local store?


- Timothy Chen


On Aug. 16, 2015, 8:31 a.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 16, 2015, 8:31 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp e43dd1c13dd4263dc326842233808ddb7a9bb74c 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 16, 2015, 8:31 a.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp e43dd1c13dd4263dc326842233808ddb7a9bb74c 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 15, 2015, 12:38 a.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Changes
---

Rebased on master.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am e990369139e7ac3b86f8b04cfd5bef559e16dd24 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp e43dd1c13dd4263dc326842233808ddb7a9bb74c 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.cpp (line 126)


Not a good pattern to have magic numbers. Maybe use a constant?



src/slave/containerizer/provisioners/docker/store.cpp (line 158)


add const qualifier ?



src/slave/containerizer/provisioners/docker/store.cpp (line 169)


extra newline?


- Jojy Varghese


On Aug. 11, 2015, 11:21 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 11, 2015, 11:21 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 11, 2015, 11:21 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
  src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 10, 2015, 10:30 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
  src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

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

(Updated Aug. 8, 2015, 1:30 a.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.


Bugs: MESOS-2849
https://issues.apache.org/jira/browse/MESOS-2849


Repository: mesos


Description
---

Stored images currently kept indefinitely.


Diffs (updated)
-

  src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
  src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 

Diff: https://reviews.apache.org/r/37197/diff/


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37197: Docker image store.

[Lily Chen]

> On Aug. 6, 2015, 9:34 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/store.hpp, line 123
> > 
> >
> > End comment with period. And why not use ImageName as keys, or 
> > basically everywhere else in the provisioner?

ImageName had no hash function, also images functionality will be moved to 
reference store.


- Lily


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


On Aug. 6, 2015, 10:51 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 10:51 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

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



src/slave/containerizer/provisioners/docker/store.hpp (line 125)


Layers no longer in store.



src/slave/containerizer/provisioners/docker/store.hpp (line 128)


fetcher is actually never used, maybe I should just get rid of it


- Lily Chen


On Aug. 6, 2015, 10:51 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 10:51 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.cpp (line 403)


prefer using explicit captures.


- Jojy Varghese


On Aug. 6, 2015, 10:51 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 10:51 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.cpp (line 169)


Why extra newline?


- Jojy Varghese


On Aug. 6, 2015, 10:51 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 10:51 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.hpp (line 72)


Why not use the new c++11 "delete" keyword?


- Jojy Varghese


On Aug. 6, 2015, 10:51 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 10:51 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.hpp (line 54)


I think new files should have javadoc style comments.


- Jojy Varghese


On Aug. 6, 2015, 10:51 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 10:51 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/store.hpp (line 128)


What is the ownership model of Fetcher member? How do you know that the 
pointer is valid through the life of StoreProcess object?


- Jojy Varghese


On Aug. 6, 2015, 10:51 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 10:51 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/store.hpp (line 123)


End comment with period. And why not use ImageName as keys, or basically 
everywhere else in the provisioner?



src/slave/containerizer/provisioners/docker/store.hpp (line 125)


What hash is this?
end comment with period too.



src/slave/containerizer/provisioners/docker/store.cpp (line 264)


Non-zero exit for tar subprocess for uri '" + uri + "', error: " +  
stringify(status.get())


- Timothy Chen


On Aug. 6, 2015, 8:34 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 6, 2015, 8:34 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>