----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30037/#review71176 -----------------------------------------------------------
Several minor things, but I think it looks really solid otherwise. include/mesos/fetcher/fetcher.proto <https://reviews.apache.org/r/30037/#comment116819> What if we want to have a future "prefetch" action that downloads to cache, but doesn't need a work_directory? Maybe this should be `optional` too? It takes a full 2 releases to make a `required` field `optional`, so better to err on the side of `optional` if there might ever be a reason to not require the field (or potentially change its name/datatype) src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/30037/#comment116821> Maybe s/workDirectory/sandboxDirectory/ or just 'sandbox', to prevent confusion with the --work_dir parameters src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/30037/#comment116822> Comment, please src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/30037/#comment116839> I don't see anywhere we actually use CacheFile.key. Is that in an upcoming patch? src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116823> No need for the '&', I think, since you're defining an actual constant value here. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116825> Should we be chowning the cacheDir here instead of workDir? Or both? src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116826> Remove this redundant TODO, since you have another, more detailed TODO below. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116827> Does anybody set the 'caching' field yet (after all your patches)? Or are we defaulting everywhere to false for the first release, making framework authors explicitly opt-in for now? I suppose without hashing, Mesos can't assume that the uri hasn't changed since being cached, so we'll have to let the framework make that decision. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116828> There's nothing asynchronous between these two continuations. Might as well merge them? src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116830> s/but we do need/so we need/ src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116831> Grammar nit: "tighter limits _on_ X than on Y" or "tighter limits regarding X _vs_ Y" Should probably also choose one of "how many..." vs. "the number of...", to provide better parallels between the two things you're comparing. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116833> Why does this even matter? Should we ever extract inside the cache dir? Even if we do, we're still trying to copy the original (unextracted) basename into the sandbox, and then extracting there, right? src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116832> s/URIs/URI's/ src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116834> If you have to wrap a function declaration/definition at all, go ahead and give each parameter its own line. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116835> const string& src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116836> const string& src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116838> How about returning and Error-type Try<Nothing> instead of doing a hard CHECK here? src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment116837> I'd suggest `const string&`, but I think you could just as easily merge these into one line: `return cache.get(cacheKey(user, uri));`, at which point I wonder why it even needs to be a method, since it's only called once. - Adam B On Jan. 28, 2015, 9:33 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30037/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2015, 9:33 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and > Timothy Chen. > > > Bugs: MESOS-2069 > https://issues.apache.org/jira/browse/MESOS-2069 > > > Repository: mesos > > > Description > ------- > > Extends the fetcher info protobuf with "actions" (fetch directly, through > cache, retrieve from cache). Switches the basis for dealing with uris to > "items", which contain the uri, the action, and potentially and cache file > name. Refactors fetch() and run(), so there is only one of each. Introduces > about half of the actual cache logic, including a hashmap of cache file > objects for bookkeeping and basic operations on it. > > > Diffs > ----- > > include/mesos/fetcher/fetcher.proto > facb87b92bf3194516f636dcc348e136af537721 > src/launcher/fetcher.cpp fed0105946da579a38357a30e7ae56e646e05b89 > src/slave/containerizer/fetcher.hpp > 1db0eaf002c8d0eaf4e0391858e61e0912b35829 > src/slave/containerizer/fetcher.cpp > d290f95251def3952c5ee34f600e1d71467f6293 > > Diff: https://reviews.apache.org/r/30037/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Bernd Mathiske > >
