> On Jan. 13, 2015, 7:55 a.m., Alexander Rukletsov wrote: > > src/slave/containerizer/fetcher.cpp, lines 186-187 > > <https://reviews.apache.org/r/29560/diff/3/?file=808632#file808632line186> > > > > `fetcher_cache_dir` is not optional, while `cache_directory` is. Maybe > > we should consider making the former optional as well?
I will make cache_directory required. > On Jan. 13, 2015, 7:55 a.m., Alexander Rukletsov wrote: > > src/slave/containerizer/fetcher.cpp, line 495 > > <https://reviews.apache.org/r/29560/diff/3/?file=808632#file808632line495> > > > > 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? At some point I will add code that creates the dir. Not an issue as long as with these earlier patches users still only exercise the legacy FETCH action without using the cache. > On Jan. 13, 2015, 7:55 a.m., Alexander Rukletsov wrote: > > src/slave/flags.hpp, line 449 > > <https://reviews.apache.org/r/29560/diff/3/?file=808635#file808635line449> > > > > 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? See above. The dir will be created on demand when exercising the cache. If that fails, fetching fails. But if caching is never used, the dir is not attempted to be created and no harm is done. This slave parameter/flag needs to be always present, because the slave cannot foresee that no task will ever use the fetcher cache. On the contrary, it needs to be prepared for tasks wanting to use the cache. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29560/#review67865 ----------------------------------------------------------- On Jan. 13, 2015, 5:35 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29560/ > ----------------------------------------------------------- > > (Updated Jan. 13, 2015, 5:35 a.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 > >
