Re: Review Request 34142: AppC provisioner.

[Ian Downes <ia...@gmail.com>]

> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, line 50
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line50>
> >
> >     nit: return type could be const?

Why? I don't see what it would add?


> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, lines 60-62
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line60>
> >
> >     If we say the dependency itself "is an" AppcImage that "has n" AppcImage(dependencies). We can kill the dependency type?
> >     
> >     AppcImage could then recursively resolve the dependencies on its own, using the composite pattern described above, i.e., AppcProvisionerProcess simply needs to call rootImage.resolve(), who loops over dependent images and calls image.resolve().
> >     
> >     We can then have fetch, fetchDependencies (not needed anymore), fetchURIs and match togerther go into AppcImage::resolve. AppcImage will now need functionality provided by discover and store; AppcPP only uses backend and does mounts.

The distinction is that AppcImage::Dependency is something that is desired, and is specified by a name plus possibly labels and a hash, but has not been realized (fetched). An AppcImage only refers to an actual image that has been fetched and its properties come from the actual image data (extracted from the json manifest and or by computing the hash).

AppcImage(::Dependency) is meant to be a simple data structure, the coordination of the discovery, fetch and store is coordinated by the provisioner.


> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, line 101
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line101>
> >
> >     nit: newline after ','

std::string, std::string are the template parameters for the hashmap, i.e., the whole thing forms the type for labels.


> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, lines 74-75
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line74>
> >
> >     const-able?

Code refactored to remove this.


> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, lines 135-139
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line135>
> >
> >     some of these should be const?

Code refactored to remove this.


- Ian


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


On May 12, 2015, 5:48 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> -----------------------------------------------------------
> 
> (Updated May 12, 2015, 5:48 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 
>   src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
> 
> Diff: https://reviews.apache.org/r/34142/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>