> On Aug. 28, 2015, 7:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 79-97
> > <https://reviews.apache.org/r/37881/diff/1/?file=1057722#file1057722line79>
> >
> >     Some high level comments.
> >     
> >     I think it will be beneficial if we can move the fetch and discovery 
> > logic to the Store. For the MVP, we don't have to impl. that in the Store.
> >     
> >     The Store only has one interface, which is the 'get' method. 'get' 
> > takes an Image::Appc and returns a vector of paths to layers. THis 
> > semantics is more natural and easy to understand: get layers for my image, 
> > if some layers are already cached, return them directly, otherwise, 
> > discover it and fetch it.
> >     
> >     The job of the provisioner is to get layers from the Store and stack 
> > them into a rootfs using the backend. It also manages the rootfs 
> > directories.
> >     
> >     To me, this is a more natural split of functionalities among 
> > components. In the future, we can also potentially unify the impl. of the 
> > provisioner (i.e., one single provisioner for both Docker and Appc images). 
> > All the image specific details are hiden in the Store interface.
> >     
> >     Thoughts?
> 
> Jiang Yan Xu wrote:
>     I am OK the this direction but a few points:
>     
>     1) I think a "dumb" store that only returns what it has and stores what 
> is given to it is natual too. (So how about stripping the fetching logic from 
> the store and put it in the provisioner? This way the provisioner is the 
> "manager" that controls all the dumb single-purpose helpers.)
>     
>     2) Unifying the provisioner would be nice but one of the remaining 
> obstacles is that we need to make sure we have a common "provisioner 
> direcotry layout", otherwise they need to be further extracted out from the 
> provisioner and the provisioner becomes so thin that there is no point in 
> having a unified provisioner anymore. --> So I am not totally sure if this 
> will happen.
>     
>     3) In the future there may be a lot of more logic required for the new 
> Store if it is responsible for doing all the heavy-lifting: cache eviction, 
> P2P fetching, registering new provision requests with ongoing fetching 
> processes, etc. But I guess this is not an argument for keeping these in the 
> provisioner, we should create other single purpose class to handle them, we 
> just need to design a Store API that's takes these into consideration.
>     
>     For now since none of these aformentioned functionality is necessary for 
> our "read-only, no dependency" provisioner let's find a way to check this as 
> long as it doesn't make it hard to refactor in the future.

Those are valid points! Let me express my rationale in some depth:

Store manages store_dir and privisioner manages rootfs_dir. A natural split is 
that: any thing involved in updating 'store_dir' should be handled by Store and 
anything involved in updating 'rootfs_dir' should be handled by rootfs_dir.

In that sense, fetching and caching should be handled by the Store. As you 
said, you can introduce sub-component in 'Store' to dealing with Discovery, 
fetching, caching, etc.

Another reason I want fetching and caching to be done in Store, independent of 
provisioner, is that: maybe, in the future, we can support 'pre-fetching' which 
is not associated with any provisioning at all. Putting all such logics in the 
provisioner will complicate it a lot. A component with less dependency is 
definitely more managable.


- Jie


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


On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
> 
> (Updated Aug. 28, 2015, 9:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
>     https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> -------
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Reply via email to