> 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? > > Bernd Mathiske wrote: > I will make cache_directory required. > > Alexander Rukletsov wrote: > This may be not the best solution: once the field is marked as required, > it will stick forever and cannot be changed back to optional. Since cache > directory doesn't feel like an absolutely "must have" (the caching may be not > used, according to your comment below), maybe it makes sense to keep the > protobuf field optional?
Agreed. It will be allowed to omit the field in case a fetcher run never uses the cache. Back to making cahce_directory optional. However, the slave cannot foresee that all fetcher runs will comply with this. Therefore specifying the cache dir remains mandatory, if only by untouched default value. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29560/#review67865 ----------------------------------------------------------- On Jan. 14, 2015, 2:58 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29560/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2015, 2:58 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 5007c0c58c5573ef19163c2c58cc3a7c5b555f46 > src/launcher/fetcher.cpp fed0105946da579a38357a30e7ae56e646e05b89 > 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 > 0bcf5ce7cfab470cabd3af3535344d19cb33b1c8 > src/slave/flags.hpp f1b8dfbb7391167b67a9498561742aa9ab9089a6 > 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 > >
