> On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, line 53 > > <https://reviews.apache.org/r/30037/diff/11/?file=839000#file839000line53> > > > > 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)
Historically, this has always been required. But I see your reasoning and since we are breaking backwards ocmpatibility we should think from a clean slate. Optional it is. However, today's mesos-fetcher will complain and fail if this string is empty. > On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.hpp, line 168 > > <https://reviews.apache.org/r/30037/diff/11/?file=839002#file839002line168> > > > > I don't see anywhere we actually use CacheFile.key. Is that in an > > upcoming patch? yes. > On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, line 156 > > <https://reviews.apache.org/r/30037/diff/11/?file=839003#file839003line156> > > > > Should we be chowning the cacheDir here instead of workDir? Or both? The cacheDir just needs to be writable and readable by the slave, not the tasks. Should we chown it to root to protect its contents? (We could later chown specific items in it if we work with links into it to save space.) > On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, line 177 > > <https://reviews.apache.org/r/30037/diff/11/?file=839003#file839003line177> > > > > 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. Nobody is expected to set this field ever before we give the green light, which includes an entry in the doc dir of Mesos that explains what it does, and more tests. Yes, default is false and opt-in at your own risk. Hashing is a later feature (MESOS-2073). > On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 213-214 > > <https://reviews.apache.org/r/30037/diff/11/?file=839003#file839003line213> > > > > There's nothing asynchronous between these two continuations. Might as > > well merge them? In principle they could be merged, indeed, but: - checkRunStatus() will be reused later. - Simply calling it from _fetch() would require _fetch() to take an extra parameter that adds complexity. More separation of concern by leaving this out. - Dispatching is supposed to be so fast that runtime performance should not be a factor to be taken into account here (on the slave - I might see this differently on the master). > On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 472-474 > > <https://reviews.apache.org/r/30037/diff/11/?file=839003#file839003line472> > > > > 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? No, we do not copy the original to the sandbox when copying from the cache. We extract directly from the file in the cache. This saves space, lets you run more tasks. This will be explained in the documentation and in a comment next to the extract flag in mesos.proto (the latter is what I consider "fixing" this issue). We may consider adding an option that gives you the original archive in the sandbox in addition to its contents even when caching, but I expect low demand for this extra feature. > On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, line 536 > > <https://reviews.apache.org/r/30037/diff/11/?file=839003#file839003line536> > > > > How about returning and Error-type Try<Nothing> instead of doing a hard > > CHECK here? If the cache file entry in the cache table (hashmap) is not present at this point, our algorithm is broken and further behavior is undefined. So this is a hard check. You could argue this should be checked at the call site. It will, in a later patch that changes the parameters of this method. > On Feb. 5, 2015, 12:40 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 547-548 > > <https://reviews.apache.org/r/30037/diff/11/?file=839003#file839003line547> > > > > 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. We'll see if we can get rid of this method in later patches. For now, I prefer "hiding" the private field, even in the same class. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30037/#review71176 ----------------------------------------------------------- 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 > >
