> On Jan. 6, 2015, 10:34 a.m., Timothy Chen wrote: > > include/mesos/mesos.proto, line 211 > > <https://reviews.apache.org/r/29560/diff/2/?file=807840#file807840line211> > > > > cache seems to be the verb to save the uri
I believe that the preceding "extract" was a mistake. Should have been "extracting". There isn't really any other precedence for a verb that I could find, but there are lots of adjectives in use and the closest verb form to that is not infinitive or imperative, but present participle (-ing form). > On Jan. 6, 2015, 10:34 a.m., Timothy Chen wrote: > > include/mesos/fetcher/fetcher.proto, line 41 > > <https://reviews.apache.org/r/29560/diff/2/?file=807839#file807839line41> > > > > This requires the user to know that the URI is already in the cache > > right? Why not just leave the last option to be DOWNLOAD_AND_INSTALL? > > > > And what does install mean? We used to fetch and extract all uris, does > > extracting means install? The user does not (need to) track what is in the cache and what not. The MesosFetcherProcess does, though, and it communicates these actions to the external fetcher program. I could change the ordering of these enums if so desired, but why? "Downloading and installing" happens for the first fetch, "only installing" for all subsequent ones for the same URI, as long as the item is in the cache. Extracting is somewhat orthogonal to all that. There are choices when exactly this should happen if so selected, either during downloading or during installation. I am opting for the latter as default behavior. A better name for "install" would be welcome. What it means: get something from the cache and place it into the work dir as if it were downloaded directly from the source. > On Jan. 6, 2015, 10:34 a.m., Timothy Chen wrote: > > src/slave/containerizer/fetcher.cpp, line 508 > > <https://reviews.apache.org/r/29560/diff/2/?file=807844#file807844line508> > > > > Why not just open getCacheFile, and if they need the file name it will > > be part of the CacheFile struct? > > > > It seems more cleaner that way. And even if we want to expose a > > getCacheFilename method, it should at least return option instead of empty > > string Let's look at this again in the next iteration (MESOS-2057). I deleted this so far unused method for now. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29560/#review66871 ----------------------------------------------------------- On Jan. 6, 2015, 7:50 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29560/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2015, 7:50 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 has been eliminated. It was not deemed essential to > debugging. > > Extended the FetcherInfo proto, the parameter aggregate for the fetcher > program. Introduced 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. > > Remodelled 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. > > Skipped implementing cache functionality in launcher/fetcher.cpp for now, > since it won't do anything until MESOS-2057, which is next, and both reviews > are more balanced in size this way. > > 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 > >
