----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29560/#review67865 -----------------------------------------------------------
src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/29560/#comment111959> `fetcher_cache_dir` is not optional, while `cache_directory` is. Maybe we should consider making the former optional as well? src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/29560/#comment111961> It looks like if the `Flags::fetcher_cache_dir` points to a non-existent dir, we return a path that doesn't exist. Can it be an issue? src/slave/flags.hpp <https://reviews.apache.org/r/29560/#comment111958> Do we require this parameter to be always present? If no, let's make it `Optional<>`, if yes, maybe it makes sense to check that the directory is present and accessible? - Alexander Rukletsov On Jan. 13, 2015, 1:35 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29560/ > ----------------------------------------------------------- > > (Updated Jan. 13, 2015, 1:35 p.m.) > > > Review request for mesos, Adam B and Benjamin Hindman. > > > Bugs: MESOS-2069 > https://issues.apache.org/jira/browse/MESOS-2069 > > > Repository: mesos-git > > > Description > ------- > > More refactoring, resolving overloading of the Fetcher::fetch() and > Fetcher::run() methods. To achieve this, the capability to redirect fetcher > stderr/out from tests is eliminated. It is not deemed essential to debugging. > > Extends the FetcherInfo proto, the parameter aggregate for the fetcher > program. Introduces essential parts and pieces, e.g. cache directory, fetcher > actions, to be built out later (MESOS-2057). > > Moved construction of the fetcher parameters (now FetcherInfo) into the fetch > method and thereupon eliminated all fetcher environment tests, since building > the environment is trivial now. > > Remodels some fetcher tests to be more "black-box-like", calling the fetcher > process more indirectly, because essentially a lot of code would just be > copied from the fetcher implementation to the tests, otherwise. Now all > fetcher tests follow the same, simpler pattern. > > We skip implementing cache functionality in launcher/fetcher.cpp for now. It > will come in a separate, dependent patch > (https://reviews.apache.org/r/29809/). > > Note some TODO(bernd-mesos) items. Where these appear, MESOS-2057 will fill > in the missing details. For example, there is good reason for > Fetcher::_fetch(). It will call run() and checkRunStatus() then. > > The fetcher needs to get a hold of the slave ID from somewhere. The chosen > approach here is to hand it down as a parameter, because this follows > prevalent patterns. However, in principle this leaves room for misuse in that > the same fetcher could potentially be used with two different slaves (which > would cause various bugs). Alternatively, one could store the slave ID in the > fetcher at some point. But that would be subject to a race. So neither > approach is ideal. > > > Diffs > ----- > > include/mesos/fetcher/fetcher.proto > facb87b92bf3194516f636dcc348e136af537721 > include/mesos/mesos.proto 540071db64961466eb75c779b3ea6863f4594437 > src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88 > src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7 > src/slave/containerizer/fetcher.hpp > 1db0eaf002c8d0eaf4e0391858e61e0912b35829 > src/slave/containerizer/fetcher.cpp > 5993670f7899233efa1e6acef4b0c7856e32f748 > src/slave/containerizer/mesos/containerizer.hpp > 802988c90ac872b0cefa5e28f06e6fec98e8d032 > src/slave/containerizer/mesos/containerizer.cpp > 5c014ebe360b9527b3edd505d47e57a4d5ce5c52 > src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef > src/tests/docker_containerizer_tests.cpp > 2105ae2c410f01e7e0d10241d5c00df143fd3439 > src/tests/fetcher_tests.cpp 8c0b0757eb388f1684d8b94393983f1844a769a7 > > Diff: https://reviews.apache.org/r/29560/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Bernd Mathiske > >
