> On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote: > > include/mesos/fetcher/fetcher.proto, line 38 > > <https://reviews.apache.org/r/30037/diff/1/?file=825117#file825117line38> > > > > These names seem to change everytime I lookat this. > > > > FETCH_DIRECTLY sounds exactly the same meaning as just FETCH though? > > > > Since there is FETCH_AND_CACHE already, shouldn't FETCH convey we're > > not caching?
You are right. Either "fetch" is anything and everything or it is only the downloading part and I mixed this up here. I intended "fetch" to be whatever gets something into the work dir, no matter which path is taken. How about DOWNLOAD_DIRECTLY, DOWNLOAD_AND_CACHE, RETRIEVE_FROM_CACHE and "fetching" is any of the above? That's what I am going to use for now. Suggestions remaining welcome. > On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote: > > include/mesos/fetcher/fetcher.proto, line 54 > > <https://reviews.apache.org/r/30037/diff/1/?file=825117#file825117line54> > > > > optional because we don't always cache right? > > probably worth commenting. Good idea to add a comment that desribes this. Will do that. > On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote: > > src/slave/containerizer/fetcher.cpp, line 169 > > <https://reviews.apache.org/r/30037/diff/1/?file=825120#file825120line169> > > > > I wonder if we can refactor this 3 level if else. > > > > It's a bad smell looking at this since if we add another action it > > becomes another nesting downwards. Let's wait with this refactoring until we have the final version in MESOS-2057! This code stretch will change for sure until then. Then indeed, this must be well-factored. I will make a note not to forget it. > On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote: > > src/slave/containerizer/fetcher.cpp, line 177 > > <https://reviews.apache.org/r/30037/diff/1/?file=825120#file825120line177> > > > > how do we know file.get().get() will succeed? > > we should at least do CHECK_SOME(file.get()) The first get() is guarded by a condition that tests for isNone(), the second is an owned pointer dereferentiation. > On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote: > > src/slave/containerizer/fetcher.cpp, line 185 > > <https://reviews.apache.org/r/30037/diff/1/?file=825120#file825120line185> > > > > Seems like the legacy behavior is that there is no caching and everyone > > downloads its own copy. > > > > Now if someone any downloads the same file the same time it instead > > returns a Failure (does this fail the task too?) > > > > I wonder if we should at least exhibit the same behavior if we don't > > support concurrent fetching, at least we shouldn't make a task fail if > > somehow their tasks are donwloading the same file at once. I prefer not to implement any behavior here that gets deleted in the next step. That would be wasted engineering. Let's just assume nobody is trying to use the cache at all until we give the green light. This code here is a stepping stone in the spirit of keeping reviews small. It is impossible to create a working cache AND keep the single review request small. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30037/#review68645 ----------------------------------------------------------- On Jan. 19, 2015, 8:05 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30037/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2015, 8:05 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-git > > > 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 > 5993670f7899233efa1e6acef4b0c7856e32f748 > > Diff: https://reviews.apache.org/r/30037/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Bernd Mathiske > >
