----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21316/#review47490 -----------------------------------------------------------
First quick pass over this code. Didn't get into the new fetcher class or tests at all. Since this is such a huge patch, here are some suggestions for breaking it up and slimming it down. - Remove unnecessary whitespace/blank-line changes. They just clog the review and arguably don't make the code any more readable. Remove any other cleanup that isn't directly related to your feature. - Maybe you could refactor the current fetcher (without caching) into a separate subprocess as a preliminary patch, to simplify the follow-up patch of fetcher-caching. - Remove EXTERNAL_FETCHER and do in subsequent review include/mesos/mesos.proto <https://reviews.apache.org/r/21316/#comment83365> Where did '2' go? Looks like you don't do anything different for EXTERNAL_FETCHER_CACHE, so let's remove it for now. You can keep the enum so it's easier to add later. include/mesos/mesos.proto <https://reviews.apache.org/r/21316/#comment83366> "If the will be" seems like you're missing part of the comment include/mesos/mesos.proto <https://reviews.apache.org/r/21316/#comment83369> Remove blank line from within comment block include/mesos/scheduler.hpp <https://reviews.apache.org/r/21316/#comment83364> Put these in a separate review, or remove them entirely. Others seem able to build on Mac without this. src/launcher/fetcher.cpp <https://reviews.apache.org/r/21316/#comment83374> "The whole program" meaning the "fetcher process"? Or the slave? src/launcher/fetcher.cpp <https://reviews.apache.org/r/21316/#comment83393> Why return here when you can just fall through to the end of the function? src/slave/constants.hpp <https://reviews.apache.org/r/21316/#comment83378> Add your new constant to the end, since there seems to be no other logical ordering here. src/slave/containerizer/mesos_containerizer.hpp <https://reviews.apache.org/r/21316/#comment83389> alphabetize src/slave/containerizer/mesos_containerizer.hpp <https://reviews.apache.org/r/21316/#comment83390> Why friended? src/slave/containerizer/mesos_fetcher.hpp <https://reviews.apache.org/r/21316/#comment83391> We don't do 'using foo' in hpp's. src/slave/containerizer/mesos_fetcher.hpp <https://reviews.apache.org/r/21316/#comment83392> Why do we even need chars for each action when we have an enum already? src/slave/flags.hpp <https://reviews.apache.org/r/21316/#comment83382> How about /tmp/mesos/cache or something? Let's at least keep it all in /tmp/mesos to make cleanup easier. That's where slave sandboxes, slave checkpoints, and a master's registry all already live. src/slave/flags.hpp <https://reviews.apache.org/r/21316/#comment83383> Rather than speak about the "duration" for which the downloads "linger", let's make it an expiration timeout, after which time unused downloads will be removed. src/slave/slave.hpp <https://reviews.apache.org/r/21316/#comment83385> Please change your chevron (">>") to "> >" for C++03 src/slave/slave.cpp <https://reviews.apache.org/r/21316/#comment83388> Why do you have to zap the cache upon recovery? Because you're not using md5's so you have to keep all your state in memory? Seems undesirable. - Adam B On June 9, 2014, 3:56 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21316/ > ----------------------------------------------------------- > > (Updated June 9, 2014, 3:56 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-336 > https://issues.apache.org/jira/browse/MESOS-336 > > > Repository: mesos-git > > > Description > ------- > > The first cut at fetcher caching. See MESOS-336 JIRA for explanation for this > approach: keep the cache info in the MesosContainerizerProcess in the save, > leverage actor single-threadedness to deal with concurrency issues without > head ache. > > Features so far: > - If URI flag "fetched_externally" (default: false) is set, the fetcher does > what it did in Mesos 0.18 and before. > - If URI flag "cached" (default: false) is not set, the fetcher also fetches > every time as in Mesos 0.18 and before. > - If URI flag "cached" is set, the UIR is only fetched once and all > subsequent fetch attempts copy from the cache file. > - URIs are cached separately per framework (ID). > - Recovery is implemented by simply wiping the entire cache. > - GC for cache files. Global flag sets lifetime after last use. Default is 1 > hour. > > Potential future features: > - symlinks instead of copying > - extraction directly from URI, without cache file > - combine that with symlinks > - Refreshing, explicit cache invalidation > - ... > > > Diffs > ----- > > include/mesos/mesos.proto 62f69d2 > include/mesos/scheduler.hpp d224945 > src/Makefile.am 4a3f2e1 > src/launcher/fetcher.cpp c4425eb > src/local/local.cpp 5d26aff > src/slave/constants.hpp ace4590 > src/slave/constants.cpp 51f65bb > src/slave/containerizer/mesos_containerizer.hpp 1f5908a > src/slave/containerizer/mesos_containerizer.cpp 1438024 > src/slave/containerizer/mesos_fetcher.hpp PRE-CREATION > src/slave/containerizer/mesos_fetcher.cpp PRE-CREATION > src/slave/flags.hpp 15e5b64 > src/slave/slave.hpp 34687e5 > src/slave/slave.cpp 643c088 > src/tests/containerizer_tests.cpp 8ea7974 > src/tests/fetcher_tests.cpp PRE-CREATION > src/tests/slave_tests.cpp 2c8f183 > > Diff: https://reviews.apache.org/r/21316/diff/ > > > Testing > ------- > > Tests have been written, have been run successfully, and are included in the > patch. > > > Thanks, > > Bernd Mathiske > >
