> 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 > >
