You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Bernd Mathiske <be...@mesosphere.io> on 2015/04/29 22:42:56 UTC

Re: Review Request 30774: Fetcher Cache

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated April 29, 2015, 1:42 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Rebased.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On April 29, 2015, 1:46 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, lines 386-387
> > <https://reviews.apache.org/r/30774/diff/41/?file=904359#file904359line386>
> >
> >     How about still calling this s/reference/get/ and then checking the references? Also, we're deprecating and removing the return value of const &. Thus:
> >     
> >     Option<shared_ptr<Cache::Entry>>& entry =
> >       cache.get(commandUser, uri.value());
> >     
> >     if (entry.isSome()) {
> >       CHECK(entry.get()->references() > 0);
> >       ... 
> >     }

After talking about this we concluded to go with explicit referencing outside the accessor.


> On April 29, 2015, 1:46 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, lines 394-395
> > <https://reviews.apache.org/r/30774/diff/41/?file=904359#file904359line394>
> >
> >     Let's get a CHECK here too:
> >     
> >     CHECK(newEntry->references() > 0);

See above.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review78530
-----------------------------------------------------------


On April 30, 2015, 7:40 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated April 30, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review78530
-----------------------------------------------------------



include/mesos/type_utils.hpp
<https://reviews.apache.org/r/30774/#comment127347>

    Let's also 'hash_combine' the 'extract' and 'executable' information here too.



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment127351>

    Great TODO, let's point the comments in slave.cpp to this TODO please!



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment127352>

    This function no longer exists!



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment127353>

    +4 please!



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127354>

    Not used!



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127356>

    Two spaces between top-level definitions please!



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127355>

    Please comment/TODO why this is being used and that you plan to remove this once we have C++11 lambdas!



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127358>

    How about still calling this s/reference/get/ and then checking the references? Also, we're deprecating and removing the return value of const &. Thus:
    
    Option<shared_ptr<Cache::Entry>>& entry =
      cache.get(commandUser, uri.value());
    
    if (entry.isSome()) {
      CHECK(entry.get()->references() > 0);
      ... 
    }



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127357>

    +2 not +4



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127359>

    Let's get a CHECK here too:
    
    CHECK(newEntry->references() > 0);



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127360>

    Let's move this comment to the top of this function so that readers of the code have a better idea of what we're trying to accomplish sooner!



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment127365>

    Let's add another comment to remind readers why if the entry's 'completion' is still pending this implies that we need to DOWNLOAD_AND_CACHE? For example:
    
    // Since the entry is not yet "complete", i.e., 'completion().isPending()', 
    // then it must be the case that we created the entry in
    // FetcherProcess::fetch otherwise the entry should have been
    // in the cache and we would have waited for the completion in
    // FetcherProcess::fetch.



src/slave/slave.cpp
<https://reviews.apache.org/r/30774/#comment127348>

    s/recoverCache/recover/ <-- Since in the future there might be other "recovery" things that need to get done that doesn't have anything to do with the cache.
    
    Also, let's please leave a comment here that explains why we're calling a static method rather than invoking a method on an instance of a 'Fetcher' directly. Our intuition is that this will likely have to change in the future.



src/slave/slave.cpp
<https://reviews.apache.org/r/30774/#comment127350>

    Comment here too, please, thanks!


- Benjamin Hindman


On April 29, 2015, 8:42 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated April 29, 2015, 8:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On April 29, 2015, 3:41 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, line 386
> > <https://reviews.apache.org/r/30774/diff/44/?file=945694#file945694line386>
> >
> >     Kill the const &.

Killed the &. Any reason this should not be const?


> On April 29, 2015, 3:41 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, line 597
> > <https://reviews.apache.org/r/30774/diff/44/?file=945694#file945694line597>
> >
> >     Let's add some helper functions on Fetcher::Cache so that we can just get this information directly in the tests rather than this "helper" function.
> >     
> >     // Return the cache.
> >     Try<list<Path>> FetcherProcess::cacheFiles();
> >     
> >     // Returns the number of entries in the cache.
> >     size_t FetcherProcess::cacheSize();

Can we please postpone this until after refactoring fetcher injection into slave/containerizer? It will be much easier to make these member functions then. I'll put a TODO for now.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82017
-----------------------------------------------------------


On April 29, 2015, 1:42 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated April 29, 2015, 1:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On April 29, 2015, 3:41 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, lines 408-417
> > <https://reviews.apache.org/r/30774/diff/44/?file=945694#file945694line408>
> >
> >     For the future:
> >     
> >     auto futures = filter(entries, [](const auto& entry) { return entry.isSome() ? entry.get() : None(); });
> >     
> >     ;-)

I could not find a suitable filter function  in std, boost, or stout yet. Shall we create one?


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82017
-----------------------------------------------------------


On May 12, 2015, 3:43 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 12, 2015, 3:43 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82017
-----------------------------------------------------------



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment132684>

    const Bytes&
    
    ?



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment132638>

    No longer used function.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132642>

    Please replace tab with spaces.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132648>

    Kill the const &.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132657>

    Let's be explicit for now with references:
    
    Option<shared_ptr<Cache::Entry>> entry = cache.get();
    
    if (entry.isSome()) {
      entry.get()->reference();
      entries[uri] = entry.get()->completion()
        .then(defer(self(), [=]() { return entry.get(); });
    } else {
      shared_ptr<Cache::Entry> newEntry =
        cache.create(cacheDirectory, commandUser, uri);
    
      newEntry->reference();
    
      entries[uri] = async(&fetchSize, uri.value(), flags.frameworks_home)
        .then(defer(self(), 
    }



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132645>

    +2 not +4 here.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132646>

    // Wait for the URI to be downloaded into the cache (or fail).



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132643>

    .then(defer(self(), [=]() { return entry.get(); }))
    
    Then please kill the 'value' function above!



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132688>

    // NOTE: We break this into two pieces because we want to be able to __block__ an instance of ...
    return _fetch(...);



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132658>

    For the future:
    
    auto futures = filter(entries, [](const auto& entry) { return entry.isSome() ? entry.get() : None(); });
    
    ;-)



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132666>

    Let's add some helper functions on Fetcher::Cache so that we can just get this information directly in the tests rather than this "helper" function.
    
    // Return the cache.
    Try<list<Path>> FetcherProcess::cacheFiles();
    
    // Returns the number of entries in the cache.
    size_t FetcherProcess::cacheSize();



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132667>

    CHECK_READY



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132669>

    Lambda-ify! ;-)



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132668>

    Please replaces tabs with spaces.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132670>

    Lambda-ify! ;-)



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132672>

    { on newline please.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment132671>

    CHECK_SOME



src/slave/slave.cpp
<https://reviews.apache.org/r/30774/#comment132632>

    Ideally we can inject the Fetcher instance into the Slave so that we don't have this global recover operation that is actually per slave:
    
    --------------------------------------------------
    
    Fetcher fetcher(flags);
    
    Slave slave(..., &fetcher);
    
    Slave::registered(...)
    {
      ...;
      Try<Nothing> recover = fetcher->recover(slaveid);
      if (recover.is...) {
        ...;
      }
      ...;
    }
    
    --------------------------------------------------
    
    But for now, let's just s/recoverCache/recover/ since the fact that the fetcher has a cache is an implementation detail.


- Benjamin Hindman


On April 29, 2015, 8:42 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated April 29, 2015, 8:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83946
-----------------------------------------------------------


- Bernd Mathiske


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On May 15, 2015, 10:34 a.m., Bernd Mathiske wrote:
> > src/tests/fetcher_cache_tests.cpp, line 426
> > <https://reviews.apache.org/r/30774/diff/48/?file=958588#file958588line426>
> >
> >     This needs to be a pointer. Or use a simple struct. Name: FetcherCacheTest::Task
> 
> Bernd Mathiske wrote:
>     Can't use the return struct approach, since Queue turns out not to be copyable. Going with passing in pointers.

Was looking at the wrong queue. Was able to use the return struct approach.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83945
-----------------------------------------------------------


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On May 15, 2015, 10:34 a.m., Bernd Mathiske wrote:
> > src/tests/fetcher_cache_tests.cpp, line 426
> > <https://reviews.apache.org/r/30774/diff/48/?file=958588#file958588line426>
> >
> >     This needs to be a pointer. Or use a simple struct. Name: FetcherCacheTest::Task

Can't use the return struct approach, since Queue turns out not to be copyable. Going with passing in pointers.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83945
-----------------------------------------------------------


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83945
-----------------------------------------------------------



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment135004>

    This needs to be a pointer. Or use a simple struct. Name: FetcherCacheTest::Task


- Bernd Mathiske


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > Just took a cursory glance since this is a huge diff, could it have been broken apart further? We've found large diffs like this one are next to impossible to review thoroughly :)

Agreed that this is normally the case. And this had been multiple smaller diffs (see "description" above). Benh eventually asked me to combine them for relatively long review sessions that covered a lot of ground in one swoop.


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.hpp, lines 288-295
> > <https://reviews.apache.org/r/30774/diff/52/?file=966644#file966644line288>
> >
> >     We might be able to get away with more descriptive names here to avoid the need for these comments:
> >     
> >     ```
> >     Bytes capacity;
> >     Bytes reserved;
> >     unsigned long fileCounter;
> >     ```
> >     
> >     'space' seems to suggest available space (to me), whereas 'capacity' seems pretty standard as a name for this. For 'tally', I can't tell from the name what is being tallied, but if we change the name to 'reserved' I have an understanding that this is the reserved space, not necessarily occupied but reserved for a purpose.

Much better indeed. Thanks!


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.hpp, lines 297-299
> > <https://reviews.apache.org/r/30774/diff/52/?file=966644#file966644line297>
> >
> >     Hard to tell why shared_ptr here is needed rather than Shared, or just Cache::Entry directly. Is there concurrent modification happening, or?

Shared is not mutable. Are you suggesting to exchange the whole entry every time we update a field?


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.cpp, line 617
> > <https://reviews.apache.org/r/30774/diff/52/?file=966645#file966645line617>
> >
> >     No need for the stringify here and below.

Thanks! will fix


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.cpp, lines 1131-1132
> > <https://reviews.apache.org/r/30774/diff/52/?file=966645#file966645line1131>
> >
> >     CHECK_LT will print the two numbers for you :)

Thanks! will fix


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.cpp, lines 1144-1145
> > <https://reviews.apache.org/r/30774/diff/52/?file=966645#file966645line1144>
> >
> >     Seems like an odd message format, since normally a meaning follows from a ':'
> >     
> >     ```
> >     Fetcher cache space overflow - space used: 2GB, exceeds total fetcher cache space: 1GB
> >     ```
> >     
> >     Here's another format where the meaning is described after the colon:
> >     
> >     ```
> >     Fetcher cache space overflow: 2GB used vs 1GB capacity
> >     ```

Yep, that's better.


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, lines 183-188
> > <https://reviews.apache.org/r/30774/diff/52/?file=966646#file966646line183>
> >
> >     I can't tell why slave id is being passed here, is there something subtle going on?

Yes. The slaveId is needed to create per-slave cache directories. There are multiple comments about this in other places that explain this and how this will go away when we will inject the slaveId after some refactoring. I will add a comment here as well.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review86871
-----------------------------------------------------------


On May 21, 2015, 9:05 a.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 21, 2015, 9:05 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
>   include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
>   src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
>   src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 
>   src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
>   src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
>   src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
>   src/tests/docker_containerizer_tests.cpp 154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
>   src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Ben Mahler <be...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review86871
-----------------------------------------------------------


Just took a cursory glance since this is a huge diff, could it have been broken apart further? We've found large diffs like this one are next to impossible to review thoroughly :)


src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment139000>

    We might be able to get away with more descriptive names here to avoid the need for these comments:
    
    ```
    Bytes capacity;
    Bytes reserved;
    unsigned long fileCounter;
    ```
    
    'space' seems to suggest available space (to me), whereas 'capacity' seems pretty standard as a name for this. For 'tally', I can't tell from the name what is being tallied, but if we change the name to 'reserved' I have an understanding that this is the reserved space, not necessarily occupied but reserved for a purpose.



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment139052>

    Hard to tell why shared_ptr here is needed rather than Shared, or just Cache::Entry directly. Is there concurrent modification happening, or?



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment139051>

    No need for the stringify here and below.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment139053>

    CHECK_LT will print the two numbers for you :)



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment139055>

    Seems like an odd message format, since normally a meaning follows from a ':'
    
    ```
    Fetcher cache space overflow - space used: 2GB, exceeds total fetcher cache space: 1GB
    ```
    
    Here's another format where the meaning is described after the colon:
    
    ```
    Fetcher cache space overflow: 2GB used vs 1GB capacity
    ```



src/slave/containerizer/mesos/containerizer.hpp
<https://reviews.apache.org/r/30774/#comment139050>

    I can't tell why slave id is being passed here, is there something subtle going on?


- Ben Mahler


On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 21, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
>   include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
>   src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
>   src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 
>   src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
>   src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
>   src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
>   src/tests/docker_containerizer_tests.cpp 154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
>   src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Jie Yu <yu...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review87615
-----------------------------------------------------------



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment140008>

    Any reason not using AWAIT_READY? CHECK_READY will abort the process while AWAIT_READY will just abort the test.
    
    ```
    19:53:22 DEBUG: F0611 19:53:23.743101 60646 fetcher_cache_tests.cpp:354] CHECK_READY(offers): is PENDING Failed to wait for resource offers
    19:53:22 DEBUG: *** Check failure stack trace: ***
    19:53:22 DEBUG:     @     0x7f431c3a644d  google::LogMessage::Fail()
    19:53:22 DEBUG:     @     0x7f431c3a828d  google::LogMessage::SendToLog()
    19:53:22 DEBUG:     @     0x7f431c3a603c  google::LogMessage::Flush()
    19:53:22 DEBUG:     @     0x7f431c3a8b89  google::LogMessageFatal::~LogMessageFatal()
    19:53:22 DEBUG:     @           0x53d9b8  _CheckFatal::~_CheckFatal()
    19:53:22 DEBUG:     @           0x66c26f  mesos::internal::tests::FetcherCacheTest::launchTask()
    19:53:22 DEBUG:     @           0x66fb09  mesos::internal::tests::FetcherCacheTest_CachedFallback_Test::TestBody()
    19:53:22 DEBUG:     @           0xbb1db3  testing::internal::HandleExceptionsInMethodIfSupported<>()
    19:53:22 DEBUG:     @           0xba9057  testing::Test::Run()
    19:53:22 DEBUG:     @           0xba90fe  testing::TestInfo::Run()
    19:53:22 DEBUG:     @           0xba9205  testing::TestCase::Run()
    19:53:22 DEBUG:     @           0xba94a8  testing::internal::UnitTestImpl::RunAllTests()
    19:53:22 DEBUG:     @           0xba9747  testing::UnitTest::Run()
    19:53:22 DEBUG:     @           0x4a1dc3  main
    19:53:22 DEBUG:     @     0x7f431a1d7d5d  __libc_start_main
    19:53:22 DEBUG:     @           0x4ad109  (unknown)
    19:53:23 DEBUG: make[3]: *** [check-local] Aborted (core dumped)
    19:53:23 DEBUG: make[3]: Leaving directory `/builddir/build/BUILD/mesos-0.23.0/src'
    19:53:23 DEBUG: make[2]: *** [check-am] Error 2
    19:53:23 DEBUG: make[2]: Leaving directory `/builddir/build/BUILD/mesos-0.23.0/src'
    19:53:23 DEBUG: make[1]: *** [check] Error 2
    19:53:23 DEBUG: make[1]: Leaving directory `/builddir/build/BUILD/mesos-0.23.0/src'
    19:53:23 DEBUG: make: *** [check-recursive] Error 1
    19:53:23 DEBUG: RPM build errors:
    ```


- Jie Yu


On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 21, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
>   include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
>   src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
>   src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 
>   src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
>   src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
>   src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
>   src/tests/docker_containerizer_tests.cpp 154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
>   src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review84857
-----------------------------------------------------------

Ship it!



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment136275>

    This looks like a good candidate for inlining too.



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment136270>

    Dead code?



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment136271>

    Dead code?



src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/30774/#comment136272>

    These two can easily and cleanly be inlined via lambdas.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment137788>

    A good candidate for a C++11 lambda.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment137789>

    Another candidate for a C++11 lamdba!



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment136273>

    Why the body of this is not inlined should be commented here otherwise someone is likely wondering what was the reason for not inlining `_fetch` here.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment137790>

    Why is os::chown no longer done any more? If we don't need to chown, why? And does that mean we can clean up this code considerably as the comment suggests?



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment137792>

    What about closing 'out' and 'err' here now?



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment137793>

    And need to close 'out' and 'err' here too.



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment136274>

    This comment needs updating, looks like when you removed `__run` you forgot to do a global search for all instances of `__run`?



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment137794>

    CHECK_EQ(space, bytes);



src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/30774/#comment137795>

    Is this really possibly anymore given the current design where 'adjust' will fail if the size we downloaded was bigger than what we first determined via 'fetchSize'?



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment137797>

    Please put '{' on newline and this could really use a comment!



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment137805>

    I'd like us to elaborate the comment here and add a TODO that we want a generic HTTP server for use in tests that has functionality like pausing requests. I'm not a huge fan of the half-actor half not strategy here, it's not something we want others to replicate and we should explicitly call that out so we don't get more broken windows.



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment137803>

    This might break if someone has a machine configured where the default IP is not 127.0.0.1, but everything should stringify correctly for you anyway:
    
    return "http://" + stringify(self().address) + "/" + self().id + "/";



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment137806>

    Using locks here is dangerous because on a smaller machine depending on the number of requests we have come through a test we might actually deadlock the entire process. We should call this out explicitly and leave a TODO on how we can do this asynchronously.


- Benjamin Hindman


On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 21, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
>   include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
>   src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
>   src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 
>   src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
>   src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
>   src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
>   src/tests/docker_containerizer_tests.cpp 154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
>   src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated May 21, 2015, 9:05 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

memory::shared_ptr -> std::shared_ptr. This allows removing #include <stout/memory.hpp>, which blocked make distcheck.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
  include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
  src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
  src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
  src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 
  src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
  src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
  src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
  src/tests/docker_containerizer_tests.cpp 154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
  src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated May 21, 2015, 5:06 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Fixed minor style issues.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
  include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
  src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
  src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
  src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 
  src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
  src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
  src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
  src/tests/docker_containerizer_tests.cpp 154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
  src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated May 19, 2015, 8:58 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Rebased to latest master.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
  include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
  src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
  src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
  src/slave/containerizer/mesos/containerizer.cpp 696e359de66305512eedf8e269543fafa21f4bc3 
  src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
  src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
  src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
  src/tests/docker_containerizer_tests.cpp 154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
  src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated May 15, 2015, 3:07 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Fixed remaining issues.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83944
-----------------------------------------------------------



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment135003>

    AWAIT_READY, not AWAIT_READY_FOR, coz the default is 15 and that works fine, too.


- Bernd Mathiske


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated May 13, 2015, 3:07 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Changed the way waiting for task completion is handled. Moved such waiting outside of launchTask(). Made helper functions create futures we can wait for with AWAIT_READY_FOR.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated May 12, 2015, 3:43 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Minor cleanups, mostly removing & from const assignments.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated May 4, 2015, 4:29 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Addressed most open issues. The rest will be dealt with soon.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 247
> > <https://reviews.apache.org/r/30774/diff/45/?file=947044#file947044line247>
> >
> >     Why are we doing 'driver->start();' here?

Good point, does not belong inside here. Moved it out of this method. Put extra calls at the call sites.


> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 681
> > <https://reviews.apache.org/r/30774/diff/45/?file=947044#file947044line681>
> >
> >     Capitlization please. Everywhere. ;-)

That would not be following the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Type_Names


> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 1179
> > <https://reviews.apache.org/r/30774/diff/45/?file=947044#file947044line1179>
> >
> >     Path
> >     {
> >       Try<bool> executable() const;
> >     };
> >     
> >     
> >     Path(runDirectory, commandFilename).executable();

Should we wait with this until Path has been fleshed out? This would be a separate review.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82260
-----------------------------------------------------------


On April 30, 2015, 7:40 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated April 30, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.


> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 1179
> > <https://reviews.apache.org/r/30774/diff/45/?file=947044#file947044line1179>
> >
> >     Path
> >     {
> >       Try<bool> executable() const;
> >     };
> >     
> >     
> >     Path(runDirectory, commandFilename).executable();
> 
> Bernd Mathiske wrote:
>     Should we wait with this until Path has been fleshed out? This would be a separate review.

I added a TODO and this will lead to a separate JIRA ticket to enhance Path.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82260
-----------------------------------------------------------


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82260
-----------------------------------------------------------



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132957>

    s/Option<string>(/Some(/



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132958>

    s/c/create/



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132963>

    Try<MesosContainerizer*> create =
      MesosContainerizer::create(flags, true, fetcher);



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132959>

    Newline before comment please.



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132961>

    Why are we doing 'driver->start();' here?



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132964>

    s/maxExpectedStatusUpdates/MAX_EXPECTED_STATUS_UPDATES/



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132966>

    auto await = [=](const TaskStatus& status) {
      CHECK_EQ(task.task_id(), status.task_id());
      if (status.state() == TASK_FINISHED) {
        return Nothing();
      }
      return taskStatusQueue.get()
        .then([=](const TaskStatus& status) {
          return await(status);
        });
    }
    
    Future<Nothing> finished = taskStatusQueue.get()
      .then([=](const TaskStatus& status) { return await(status); });
    
    AWAIT_READY_FOR(finished, Seconds(10));
    
    // ------------
    
    AWAIT_READY_FOR(awaitFinished(taskStatusQueue), Seconds(10));
    
    // ------------
    
    Promise<Nothing> finished;
    EXPECT_CALL(scheduler, statusUpdate(driver, _))
      .WillRepeatedly([=, &finished](
          const SchedulerDriver&, const TaskStatus& status) {
        CHECK_EQ(task.task_id(), status.task_id());
        if (status.state() == TASK_FINISHED) {
          finished.set(Nothing());
        }
      });
    
    AWAIT_READY_FOR(finished.future(), Seconds(10));
    
    // ------------------------
    
    But since we don't have C++11 lambdas as gmock actions yet, instead create your own action:
    
    ACTION_TEMPLATE(FinishedStatusUpdate, promise)
    {
      const TaskStatus& status = arg1;
      // TODO: check
      if (status.state() == TASK_FINISHED) {
        promise->set(Nothing());
      }
    }
    
    And finally, let's pull this out of s/runTask/launchTask/ so it has consistent behavior with launchTasks.



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132968>

    InvokeLambda<N>(args, lambda)
    {
      return lambda(get<0>(args), get<1>)(args), get<2>(args), ...);
    }
    
    InvokeVoidLambda<N>(args, lambda)
    {
      lambda(get<0>(args), get<1>)(args), get<2>(args), ...);
    }



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132980>

    Let's rename this to something that is more indicative of what's happening when we reach or block on a FetcherProcess::_fetch.



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132981>

    CHECK_READY
    
    Here and everywhere else please. ;-)



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132982>

    const&



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132986>

    Capitlization please. Everywhere. ;-)



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132987>

    if (strings::contains(event.request->path, COMMAND_NAME)) {
    
    }
    
    Below please too!



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132989>

    Unroll please, the switch is harder to grok below.



src/tests/fetcher_cache_tests.cpp
<https://reviews.apache.org/r/30774/#comment132988>

    Path
    {
      Try<bool> executable() const;
    };
    
    Path(runDirectory, commandFilename).executable();



src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/30774/#comment132990>

    EXPECT_ERROR
    
    (Search for 'isError())' and replace. ;-) )



src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/30774/#comment132991>

    s/..../bernd/



src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/30774/#comment132992>

    EXPECT_TRUE(strings::contains(fetch.failure(), "chown"));
    
    (How about we just search and replace .find?)



src/tests/mesos.cpp
<https://reviews.apache.org/r/30774/#comment132993>

    Let's double check whether or not this is really necessary?


- Benjamin Hindman


On May 1, 2015, 2:40 a.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> -----------------------------------------------------------
> 
> (Updated May 1, 2015, 2:40 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
>     https://issues.apache.org/jira/browse/MESOS-2057
>     https://issues.apache.org/jira/browse/MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2070
>     https://issues.apache.org/jira/browse/MESOS-2072
>     https://issues.apache.org/jira/browse/MESOS-2073
>     https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.
> 
> 30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 
> 
> 30039: Enables fetcher cache actions in the mesos fetcher program.
> 
> 30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 
> 
> 30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.
> 
> 30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.
> 
> 30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.
> 
> 30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).
> 
> 30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.
> 
> 30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
> - mesos-fetcher does not run until evictions have been successful
> - Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
> - Reservations can be partly from available space, partly from evictions. All math included :-)
> - To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
> - Size-determination requests are now synchronized, too. Only one per URI in play happens.
> - There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
> - Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 30774: Fetcher Cache

Posted by Bernd Mathiske <be...@mesosphere.io>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
-----------------------------------------------------------

(Updated April 30, 2015, 7:40 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Changes
-------

Fixed most issues, but not all. Let's talk about theremaining ones before proceeding!


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
    https://issues.apache.org/jira/browse/MESOS-2057
    https://issues.apache.org/jira/browse/MESOS-2069
    https://issues.apache.org/jira/browse/MESOS-2070
    https://issues.apache.org/jira/browse/MESOS-2072
    https://issues.apache.org/jira/browse/MESOS-2073
    https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
-------

Almost all of the functionality in epic MESOS-336. Downloaded files from CommandInfo::URIs can now be cached in a cache directory designated by a slave flag. This only happens when asked for by an extra flag in the URI and is thus backwards-compatible. The cache has a size limit also given by a new slave flag. Cache-resident files are evicted as necessary to make space for newly fetched ones. Concurrent attempts to cache the same URI leads to only one download. The fetcher program remains external for safety reasons, but is now augmented with more elaborate parameters packed into a JSON object to implement specific fetch actions for all of the above. Additional testing includes fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-----

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
-------

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when the fetcher uses JSON in a single env var as a parameter. They never tested anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of calling the run method of the fetcher directly, calling through fetch(). Also removes all uses of I/O redirection, which is not really needed for debugging, and thus the next patch can refactor fetch() and run(). (The latter comes in two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field that will later cause fetcher cache actions. Also introduces the notion of a cache directory to the fetcher info protobuf. And then propagates these additions throughout the rest of the code base where applicable. This includes passing the slave ID all the way down to the place where the cache dir name is constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly bypassing the cache, fetch through the cache, retrieve from the cache). Switches the basis for dealing with uris to "items", which contain the uri, the action, and potentially a 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. 

30039: Enables fetcher cache actions in the mesos fetcher program.

30006: Enables concurrent downloading into the fetcher cache. Reuse of download results in the cache when multiple fetcher runs occur concurrently. 

30614: This is to ensure that all this refactoring of fetcher code has not broken HDFS fetching. Adds a test that exercises the C++ code paths in Mesos and mesos-fetcher related to fetching from HDFS. Uses a mock HDFS client written in bash that acts just like a real "hadoop" command if used in the right limited way.

30124: Inserted fetcher cache zap upon slave startup, recovery and shutdown. This implements recovery in an acceptable, yet most simple way.

30173: Created fetcher cache tests. Adds a new test source file containing a test fixture and tests to find out if the fetcher cache works with a variety of settings.

30616: Adds hdfs::du() which calls "hadoop fs -du -h" and returns a string that contains the file size for the URI passed as argument. This is needed to determine the size of a file on HDFS before downloading it to the fetcher cache (to ensure there is enough space).

30621: Refactored URI type separation in mesos-fetcher. Moved the URI type separation code (distinguishes http, hdfs, local copying, etc.) from mesos-fetcher to the fetcher process/actor, since it is going to be reused by download size queries when we introduce fetcher cache management. Also factored out URI validation, which will be used the same way by mesos-fetcher and the fetcher process/actor.

30626: Fetcher cache eviction. This happens when the cache does not have enough space to accomodate upcoming downloads to the cache. Necessary provisions included here:
- mesos-fetcher does not run until evictions have been successful
- Cache space is reserved while (async) waiting for eviction to succeed. If it fails, the reservation gets undone.
- Reservations can be partly from available space, partly from evictions. All math included :-)
- To find out how much space is needed, downloading has a prelude in which we query the download size from the URI. This works for all URI types that mesos-fetcher currently supports, including http and hdfs.
- Size-determination requests are now synchronized, too. Only one per URI in play happens.
- There is cleanup code for all kinds of error situations. At the very end of the fetch attempt, each list is processed for undoing things like space reservations and eviction disabling.
- Eviction gets disabled for URIs that are currently in use, i.e. the related cache files are. We use reference counting for this, since there may be concurrent fetch attempts using the same cache files.


Thanks,

Bernd Mathiske