----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30774/#review75754 -----------------------------------------------------------
include/mesos/fetcher/fetcher.proto <https://reviews.apache.org/r/30774/#comment122998> First, the "preloading" feature does not exist so let's not comment as much. Second, if you perceive this being optional in the future we should make it be optional now. include/mesos/mesos.proto <https://reviews.apache.org/r/30774/#comment122995> Let's remove the 'force' functionality for now. Why? First, as a user this is a tedious primitive to use: it requires the scheduler to set the field when it wants a new version of the URI but there is no way for the scheduler to know after the new version has been properly cached on a particular machine. If a user always wants the latest version they should just set 'cache' to false. A better approach here would be to introduce a 'sha' for the URI that the fetcher can compare against and if the SHAs are different force the download then (but let's not do the SHA right now, we can do that as a follow up, and keep the MVP simple). Given that then we'll have a basic enum for 'Cache' and folks have already been confused by the values 'FETCH' and 'FORCE' I think we should revert to the simpler: 'optional bool cache'. What would be other potential 'enum Cache' values going forward? If we can think of good examples then the right thing to do here would probably be to introduce a 'message CacheInfo' that we embed in URI. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment122999> Can we put a newline above this to make it easier to read? The code is very compressed here with the line continuation from 'sourcePath ='. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123000> Why not 'else if'? The value of doing 'else if' here is that this block is comparing the status of the same variable so continuing with an 'else if' helps to capture that semantics. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123001> Can we please add a TODO here to refactor this into stout so that people can more easily chmod an exectuable? For example, we could define some static flags so that someone can do: os::chmod(path, EXECUTABLE_CHMOD_FLAGS); src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123004> Can we improve the error message here (and below), e.g.: return Error("Failed to determine the basename of the URI: " + basename.error()); src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123006> Can you comment the relationship between the FetcherInfo::Item and the FetcherInfo here? Is the FetcherInfo::Item within the FetcherInfo but FetcherInfo is included because you just want to get the 'sandbox_directory' and 'cache_directory' and rather than pulling those out explicitly you just passed the entire FetcherInfo? src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123012> Understanding the logic here is confusing: (1) Why do we copy the file if extracting failed? It's clear to me that this is because this is the way we've always done this, but with the new code pulled into this function we should capture this with comments for a future reader. (2) Why do we return the sandbox directory when extraction was sucessful? Why do we return the value from 'chmodExecutable'? I would have expected to test whether 'chmodExecuable' was successful and then returned whatever the semantics of this function should return. Let's comment these functions return semantics. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123025> Why are these not CHECKs? Since you're the one setting up the FetcherInfo it seems like you should know explicitly whether or not the cache_filename was set! Same for the cache_directory below as well. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123015> Newline above here to break this up from the complicated line above. src/launcher/fetcher.cpp <https://reviews.apache.org/r/30774/#comment123028> As mentioned above, it would be great to really capture the relationship between the FetcherInfo and the FetcherInfo::Item. If The FetcherInfo encapsulates the FetcherInfo::Item I would also suggest switching the order of the parameters to signify that. src/slave/flags.hpp <https://reviews.apache.org/r/30774/#comment123031> Can we make this a Path to start? src/slave/slave.cpp <https://reviews.apache.org/r/30774/#comment123202> We should do recovery on the fetcher itself: Try<Nothing> recover = fetcher->recover(flags, slaveId); It seems very weird to have a static generic Fetcher recover functionality that implies that we can't have multiple Fetchers running at the same time. How do we start multiple slaves at the same time? - Benjamin Hindman On March 7, 2015, 3:21 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30774/ > ----------------------------------------------------------- > > (Updated March 7, 2015, 3:21 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/fetcher-cache-internals.md PRE-CREATION > docs/fetcher.md PRE-CREATION > include/mesos/fetcher/fetcher.proto > 311af9aebc6a85dadba9dbeffcf7036b70896bcc > include/mesos/mesos.proto 9df972d750ce1e4a81d2e96cc508d6f83cad2fc8 > src/Makefile.am d299f07d865080676ca8a550cf6005c6ab32839f > src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b > src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 > src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec > src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88 > src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7 > src/slave/containerizer/fetcher.hpp > 1db0eaf002c8d0eaf4e0391858e61e0912b35829 > src/slave/containerizer/fetcher.cpp > 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 > src/slave/containerizer/mesos/containerizer.hpp > ae61a0fcd19f2ba808624312401f020121baf5d4 > src/slave/containerizer/mesos/containerizer.cpp > ec4626f903d44c0911093ff763ef16ad27c418a9 > src/slave/flags.hpp 56b25caf3901b38bdecb50310e8bcae0b114efa8 > src/slave/slave.cpp a06d68032f26ccb3f786b6ea7c3a6c3c52449bd2 > src/tests/docker_containerizer_tests.cpp > 06cd3d89ecbaaac17ae6970604b21fbe29f6e887 > src/tests/fetcher_cache_tests.cpp PRE-CREATION > src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e > src/tests/mesos.hpp e91e5e484eea4587ac8f2eb9cefeab4acc9f4615 > src/tests/mesos.cpp c8f43d21b214e75eaac2870cbdf4f03fd18707d1 > > 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 > >
