mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 21316: Preview of MESOS-336
Date Wed, 09 Jul 2014 08:18:40 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message