mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <a...@mesosphere.io>
Subject Re: Review Request 29560: Added basic cache data structures to the fetcher process.
Date Tue, 13 Jan 2015 15:55:07 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message