----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30037/#review68645 -----------------------------------------------------------
include/mesos/fetcher/fetcher.proto <https://reviews.apache.org/r/30037/#comment112979> 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? include/mesos/fetcher/fetcher.proto <https://reviews.apache.org/r/30037/#comment112980> optional because we don't always cache right? probably worth commenting. include/mesos/fetcher/fetcher.proto <https://reviews.apache.org/r/30037/#comment112981> why change the numbers? 4, 5 are missing now too. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112982> I think for constants, all places in our code we defined static variables instead of defining them. I propose for being consistent. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112983> Let's add an extra space before the return, we've often been doing this across the code base and make it easier to view. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112984> 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. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112987> else { src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112985> how do we know file.get().get() will succeed? we should at least do CHECK_SOME(file.get()) src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112986> 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. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112988> Wierd to see a TODO comment after a last return statement, we should pull this before the return. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112989> Let's put a space before the return src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112990> 4 spaces for method call parameters. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112992> if (dotIndex src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112991> space before return src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/30037/#comment112993> 4 spaces - Timothy Chen On Jan. 19, 2015, 4:05 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30037/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2015, 4:05 p.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 > >
