Re: Review Request 30774: Fetcher Cache

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review87615
---



src/tests/fetcher_cache_tests.cpp


Any reason not using AWAIT_READY? CHECK_READY will abort the process while 
AWAIT_READY will just abort the test.

```
19:53:22 DEBUG: F0611 19:53:23.743101 60646 fetcher_cache_tests.cpp:354] 
CHECK_READY(offers): is PENDING Failed to wait for resource offers
19:53:22 DEBUG: *** Check failure stack trace: ***
19:53:22 DEBUG: @ 0x7f431c3a644d  google::LogMessage::Fail()
19:53:22 DEBUG: @ 0x7f431c3a828d  google::LogMessage::SendToLog()
19:53:22 DEBUG: @ 0x7f431c3a603c  google::LogMessage::Flush()
19:53:22 DEBUG: @ 0x7f431c3a8b89  
google::LogMessageFatal::~LogMessageFatal()
19:53:22 DEBUG: @   0x53d9b8  _CheckFatal::~_CheckFatal()
19:53:22 DEBUG: @   0x66c26f  
mesos::internal::tests::FetcherCacheTest::launchTask()
19:53:22 DEBUG: @   0x66fb09  
mesos::internal::tests::FetcherCacheTest_CachedFallback_Test::TestBody()
19:53:22 DEBUG: @   0xbb1db3  
testing::internal::HandleExceptionsInMethodIfSupported<>()
19:53:22 DEBUG: @   0xba9057  testing::Test::Run()
19:53:22 DEBUG: @   0xba90fe  testing::TestInfo::Run()
19:53:22 DEBUG: @   0xba9205  testing::TestCase::Run()
19:53:22 DEBUG: @   0xba94a8  
testing::internal::UnitTestImpl::RunAllTests()
19:53:22 DEBUG: @   0xba9747  testing::UnitTest::Run()
19:53:22 DEBUG: @   0x4a1dc3  main
19:53:22 DEBUG: @ 0x7f431a1d7d5d  __libc_start_main
19:53:22 DEBUG: @   0x4ad109  (unknown)
19:53:23 DEBUG: make[3]: *** [check-local] Aborted (core dumped)
19:53:23 DEBUG: make[3]: Leaving directory 
`/builddir/build/BUILD/mesos-0.23.0/src'
19:53:23 DEBUG: make[2]: *** [check-am] Error 2
19:53:23 DEBUG: make[2]: Leaving directory 
`/builddir/build/BUILD/mesos-0.23.0/src'
19:53:23 DEBUG: make[1]: *** [check] Error 2
19:53:23 DEBUG: make[1]: Leaving directory 
`/builddir/build/BUILD/mesos-0.23.0/src'
19:53:23 DEBUG: make: *** [check-recursive] Error 1
19:53:23 DEBUG: RPM build errors:
```


- Jie Yu


On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 21, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
>   include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
>   src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
>   src/slave/containerize

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > Just took a cursory glance since this is a huge diff, could it have been 
> > broken apart further? We've found large diffs like this one are next to 
> > impossible to review thoroughly :)

Agreed that this is normally the case. And this had been multiple smaller diffs 
(see "description" above). Benh eventually asked me to combine them for 
relatively long review sessions that covered a lot of ground in one swoop.


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.hpp, lines 288-295
> > 
> >
> > We might be able to get away with more descriptive names here to avoid 
> > the need for these comments:
> > 
> > ```
> > Bytes capacity;
> > Bytes reserved;
> > unsigned long fileCounter;
> > ```
> > 
> > 'space' seems to suggest available space (to me), whereas 'capacity' 
> > seems pretty standard as a name for this. For 'tally', I can't tell from 
> > the name what is being tallied, but if we change the name to 'reserved' I 
> > have an understanding that this is the reserved space, not necessarily 
> > occupied but reserved for a purpose.

Much better indeed. Thanks!


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.hpp, lines 297-299
> > 
> >
> > Hard to tell why shared_ptr here is needed rather than Shared, or just 
> > Cache::Entry directly. Is there concurrent modification happening, or?

Shared is not mutable. Are you suggesting to exchange the whole entry every 
time we update a field?


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.cpp, line 617
> > 
> >
> > No need for the stringify here and below.

Thanks! will fix


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.cpp, lines 1131-1132
> > 
> >
> > CHECK_LT will print the two numbers for you :)

Thanks! will fix


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/fetcher.cpp, lines 1144-1145
> > 
> >
> > Seems like an odd message format, since normally a meaning follows from 
> > a ':'
> > 
> > ```
> > Fetcher cache space overflow - space used: 2GB, exceeds total fetcher 
> > cache space: 1GB
> > ```
> > 
> > Here's another format where the meaning is described after the colon:
> > 
> > ```
> > Fetcher cache space overflow: 2GB used vs 1GB capacity
> > ```

Yep, that's better.


> On June 5, 2015, 7:10 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, lines 183-188
> > 
> >
> > I can't tell why slave id is being passed here, is there something 
> > subtle going on?

Yes. The slaveId is needed to create per-slave cache directories. There are 
multiple comments about this in other places that explain this and how this 
will go away when we will inject the slaveId after some refactoring. I will add 
a comment here as well.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review86871
---


On May 21, 2015, 9:05 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 21, 2015, 9:05 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cach

Re: Review Request 30774: Fetcher Cache

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review86871
---


Just took a cursory glance since this is a huge diff, could it have been broken 
apart further? We've found large diffs like this one are next to impossible to 
review thoroughly :)


src/slave/containerizer/fetcher.hpp


We might be able to get away with more descriptive names here to avoid the 
need for these comments:

```
Bytes capacity;
Bytes reserved;
unsigned long fileCounter;
```

'space' seems to suggest available space (to me), whereas 'capacity' seems 
pretty standard as a name for this. For 'tally', I can't tell from the name 
what is being tallied, but if we change the name to 'reserved' I have an 
understanding that this is the reserved space, not necessarily occupied but 
reserved for a purpose.



src/slave/containerizer/fetcher.hpp


Hard to tell why shared_ptr here is needed rather than Shared, or just 
Cache::Entry directly. Is there concurrent modification happening, or?



src/slave/containerizer/fetcher.cpp


No need for the stringify here and below.



src/slave/containerizer/fetcher.cpp


CHECK_LT will print the two numbers for you :)



src/slave/containerizer/fetcher.cpp


Seems like an odd message format, since normally a meaning follows from a 
':'

```
Fetcher cache space overflow - space used: 2GB, exceeds total fetcher cache 
space: 1GB
```

Here's another format where the meaning is described after the colon:

```
Fetcher cache space overflow: 2GB used vs 1GB capacity
```



src/slave/containerizer/mesos/containerizer.hpp


I can't tell why slave id is being passed here, is there something subtle 
going on?


- Ben Mahler


On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 21, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
>   include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
>   src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3e18617b0dbac58176bfd41dc550898eb6a4a79e 
>   src/s

Re: Review Request 30774: Fetcher Cache

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review84857
---

Ship it!



src/slave/containerizer/fetcher.hpp


This looks like a good candidate for inlining too.



src/slave/containerizer/fetcher.hpp


Dead code?



src/slave/containerizer/fetcher.hpp


Dead code?



src/slave/containerizer/fetcher.hpp


These two can easily and cleanly be inlined via lambdas.



src/slave/containerizer/fetcher.cpp


A good candidate for a C++11 lambda.



src/slave/containerizer/fetcher.cpp


Another candidate for a C++11 lamdba!



src/slave/containerizer/fetcher.cpp


Why the body of this is not inlined should be commented here otherwise 
someone is likely wondering what was the reason for not inlining `_fetch` here.



src/slave/containerizer/fetcher.cpp


Why is os::chown no longer done any more? If we don't need to chown, why? 
And does that mean we can clean up this code considerably as the comment 
suggests?



src/slave/containerizer/fetcher.cpp


What about closing 'out' and 'err' here now?



src/slave/containerizer/fetcher.cpp


And need to close 'out' and 'err' here too.



src/slave/containerizer/fetcher.cpp


This comment needs updating, looks like when you removed `__run` you forgot 
to do a global search for all instances of `__run`?



src/slave/containerizer/fetcher.cpp


CHECK_EQ(space, bytes);



src/slave/containerizer/fetcher.cpp


Is this really possibly anymore given the current design where 'adjust' 
will fail if the size we downloaded was bigger than what we first determined 
via 'fetchSize'?



src/tests/fetcher_cache_tests.cpp


Please put '{' on newline and this could really use a comment!



src/tests/fetcher_cache_tests.cpp


I'd like us to elaborate the comment here and add a TODO that we want a 
generic HTTP server for use in tests that has functionality like pausing 
requests. I'm not a huge fan of the half-actor half not strategy here, it's not 
something we want others to replicate and we should explicitly call that out so 
we don't get more broken windows.



src/tests/fetcher_cache_tests.cpp


This might break if someone has a machine configured where the default IP 
is not 127.0.0.1, but everything should stringify correctly for you anyway:

return "http://"; + stringify(self().address) + "/" + self().id + "/";



src/tests/fetcher_cache_tests.cpp


Using locks here is dangerous because on a smaller machine depending on the 
number of requests we have come through a test we might actually deadlock the 
entire process. We should call this out explicitly and leave a TODO on how we 
can do this asynchronously.


- Benjamin Hindman


On May 21, 2015, 4:05 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 21, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> 

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated May 21, 2015, 9:05 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

memory::shared_ptr -> std::shared_ptr. This allows removing #include 
, which blocked make distcheck.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
  include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
  src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
  src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
3e18617b0dbac58176bfd41dc550898eb6a4a79e 
  src/slave/containerizer/mesos/containerizer.cpp 
696e359de66305512eedf8e269543fafa21f4bc3 
  src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
  src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
  src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
  src/tests/docker_containerizer_tests.cpp 
154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
  src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. 

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated May 21, 2015, 5:06 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Fixed minor style issues.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
  include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
  src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
  src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
3e18617b0dbac58176bfd41dc550898eb6a4a79e 
  src/slave/containerizer/mesos/containerizer.cpp 
696e359de66305512eedf8e269543fafa21f4bc3 
  src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
  src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
  src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
  src/tests/docker_containerizer_tests.cpp 
154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
  src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. Introduces about half of the actual cache logic, 
including a hashmap of cac

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated May 19, 2015, 8:58 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Rebased to latest master.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 9cc5782256156ed59fd4640091413b76480d939f 
  include/mesos/type_utils.hpp 837be6f1844d5fa01c0fd84a585e7ff2cc0c987b 
  src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp ed4ee19c85387882ab2e31baa5610acb8e222d50 
  src/slave/containerizer/docker.cpp 408a4435a6f11973992486eac1659beeccc4beac 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
3e18617b0dbac58176bfd41dc550898eb6a4a79e 
  src/slave/containerizer/mesos/containerizer.cpp 
696e359de66305512eedf8e269543fafa21f4bc3 
  src/slave/flags.hpp 5c57478fcfdbcbd8ac0e5c3c79809403054e96e6 
  src/slave/flags.cpp b5e25186dad36bc1306cc6ecb268aba951a18f7e 
  src/slave/slave.cpp 8e88482f41f37ce7f2559fe793565b66ac46fb35 
  src/tests/docker_containerizer_tests.cpp 
154bf981c007ebcb8e0b2fe8551defb5ea2ba063 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp a60df75350beab8d7091cbe66213ecd920942fa4 
  src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. Introduces about half of the actual cache logic, 
including a hashmap of cac

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated May 15, 2015, 3:07 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Fixed remaining issues.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp 
f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp 
c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. Introduces about half of the actual cache logic, 
including a hashmap of cache

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On May 15, 2015, 10:34 a.m., Bernd Mathiske wrote:
> > src/tests/fetcher_cache_tests.cpp, line 426
> > 
> >
> > This needs to be a pointer. Or use a simple struct. Name: 
> > FetcherCacheTest::Task
> 
> Bernd Mathiske wrote:
> Can't use the return struct approach, since Queue turns out not to be 
> copyable. Going with passing in pointers.

Was looking at the wrong queue. Was able to use the return struct approach.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83945
---


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not really needed for 
> debugging, and thus t

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On May 15, 2015, 10:34 a.m., Bernd Mathiske wrote:
> > src/tests/fetcher_cache_tests.cpp, line 426
> > 
> >
> > This needs to be a pointer. Or use a simple struct. Name: 
> > FetcherCacheTest::Task

Can't use the return struct approach, since Queue turns out not to be copyable. 
Going with passing in pointers.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83945
---


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not really needed for 
> debugging, and thus the next patch can refactor fetch() and run(). (The 
> latter comes in two varieties, which complicates matters 

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 1179
> > 
> >
> > Path
> > {
> >   Try executable() const;
> > };
> > 
> > 
> > Path(runDirectory, commandFilename).executable();
> 
> Bernd Mathiske wrote:
> Should we wait with this until Path has been fleshed out? This would be a 
> separate review.

I added a TODO and this will lead to a separate JIRA ticket to enhance Path.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82260
---


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not really ne

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83946
---


- Bernd Mathiske


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not really needed for 
> debugging, and thus the next patch can refactor fetch() and run(). (The 
> latter comes in two varieties, which complicates matters without much 
> benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
> that will later cause fetcher cache actions. Also introduces the notion of a 
> cache directory to the fetcher info protobuf. And then propagates these 
> additions throughout the rest of the code base where applicable. This 
> includes passing the slave ID all the way down to

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83945
---



src/tests/fetcher_cache_tests.cpp


This needs to be a pointer. Or use a simple struct. Name: 
FetcherCacheTest::Task


- Bernd Mathiske


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not really needed for 
> debugging, and thus the next patch can refactor fetch() and run(). (The 
> latter comes in two varieties, which complicates matters without much 
> benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
> that will later cause fetcher cache actions. Also introduces the notion of a 
> cache directory to th

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review83944
---



src/tests/fetcher_cache_tests.cpp


AWAIT_READY, not AWAIT_READY_FOR, coz the default is 15 and that works 
fine, too.


- Bernd Mathiske


On May 13, 2015, 3:07 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 13, 2015, 3:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not really needed for 
> debugging, and thus the next patch can refactor fetch() and run(). (The 
> latter comes in two varieties, which complicates matters without much 
> benefit.)
> 
> 30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
> that will later cause fetcher cache actions. Also introduces the notion of a 
> cache directory to t

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated May 13, 2015, 3:07 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Changed the way waiting for task completion is handled. Moved such waiting 
outside of launchTask(). Made helper functions create futures we can wait for 
with AWAIT_READY_FOR.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp 
f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp 
c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a ca

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On April 29, 2015, 3:41 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, lines 408-417
> > 
> >
> > For the future:
> > 
> > auto futures = filter(entries, [](const auto& entry) { return 
> > entry.isSome() ? entry.get() : None(); });
> > 
> > ;-)

I could not find a suitable filter function  in std, boost, or stout yet. Shall 
we create one?


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82017
---


On May 12, 2015, 3:43 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 12, 2015, 3:43 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not really needed for 
> debugging, and thus the next patch can refactor fetch() and r

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated May 12, 2015, 3:43 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Minor cleanups, mostly removing & from const assignments.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp 
f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp 
c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. Introduces about half of the actual cache lo

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated May 4, 2015, 4:29 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Addressed most open issues. The rest will be dealt with soon.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp 
f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp 
c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. Introduces about half of the actual cache

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 247
> > 
> >
> > Why are we doing 'driver->start();' here?

Good point, does not belong inside here. Moved it out of this method. Put extra 
calls at the call sites.


> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 681
> > 
> >
> > Capitlization please. Everywhere. ;-)

That would not be following the style guide: 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Type_Names


> On May 1, 2015, 12:17 p.m., Benjamin Hindman wrote:
> > src/tests/fetcher_cache_tests.cpp, line 1179
> > 
> >
> > Path
> > {
> >   Try executable() const;
> > };
> > 
> > 
> > Path(runDirectory, commandFilename).executable();

Should we wait with this until Path has been fleshed out? This would be a 
separate review.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82260
---


On April 30, 2015, 7:40 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 30, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On March 10, 2015, 8:35 a.m., Benjamin Hindman wrote:
> > src/launcher/fetcher.cpp, lines 403-404
> > 
> >
> > As mentioned above, it would be great to really capture the 
> > relationship between the FetcherInfo and the FetcherInfo::Item. If The 
> > FetcherInfo encapsulates the FetcherInfo::Item I would also suggest 
> > switching the order of the parameters to signify that.
> 
> Bernd Mathiske wrote:
> The main purpose here is to fetch this one particular item, not 
> everything FetcherInfo carries. FetcherInfo is a secondary parameter that 
> provides extra parameters like cache_directory, sandbox_directory, and 
> framework_home. Putting it second makes this relationship clear IMHO. Do you 
> suggest adding all these as individual parameters?
> 
> Yes, the item is included in the list of items in FetcherInfo. Shall we 
> break up FetcherInfo into several shells, the inner one without items?

Now using split up parameters instead of FetcherInfo to pass around sandbox 
dir, cache dir, frameworks home.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review75754
---


On April 30, 2015, 7:40 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 30, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 3000

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On March 19, 2015, 9:40 a.m., Timothy Chen wrote:
> > src/slave/containerizer/fetcher.cpp, line 379
> > 
> >
> > benh, what do you think of Bernd's contentionBarrier injection? 
> > commonly we always just mock the callback (_fetch in this case) in tests to 
> > block, but Bernd wanted to introduce a specific empty method for tests. I 
> > told him this is not a pattern we use in Mesos, but like to see what you 
> > think.
> 
> Bernd Mathiske wrote:
> Of course I will stick to the prevalent patterns unless you start liking 
> this one :-)
> 
> Bernd Mathiske wrote:
> Turns out there is no method that offers itself opportunistically for the 
> prevalent pattern. The next call up is AFTER waiting for the futures that the 
> barrier needs to be BEFORE. Suggestions?
> 
> Alternatives (without judging them):
> - Factor out the loop that gathers the futures and make it a method that 
> gets called once. Then mock this method and have it dual-purposed as 
> contention barrier. 
> - Make the futures globally visible, await them in the test, too.
> - Use an explicit lock instead of mocking.

Another option has been implemented in the latest patch: cut off the lower half 
of fetch() and call it _fetch(), then mock the latter. This variant ressembles 
existing other code the most.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review77057
---


On April 30, 2015, 7:40 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 30, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 

Re: Review Request 30774: Fetcher Cache

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82260
---



src/tests/fetcher_cache_tests.cpp


s/Option(/Some(/



src/tests/fetcher_cache_tests.cpp


s/c/create/



src/tests/fetcher_cache_tests.cpp


Try create =
  MesosContainerizer::create(flags, true, fetcher);



src/tests/fetcher_cache_tests.cpp


Newline before comment please.



src/tests/fetcher_cache_tests.cpp


Why are we doing 'driver->start();' here?



src/tests/fetcher_cache_tests.cpp


s/maxExpectedStatusUpdates/MAX_EXPECTED_STATUS_UPDATES/



src/tests/fetcher_cache_tests.cpp


auto await = [=](const TaskStatus& status) {
  CHECK_EQ(task.task_id(), status.task_id());
  if (status.state() == TASK_FINISHED) {
return Nothing();
  }
  return taskStatusQueue.get()
.then([=](const TaskStatus& status) {
  return await(status);
});
}

Future finished = taskStatusQueue.get()
  .then([=](const TaskStatus& status) { return await(status); });

AWAIT_READY_FOR(finished, Seconds(10));

// 

AWAIT_READY_FOR(awaitFinished(taskStatusQueue), Seconds(10));

// 

Promise finished;
EXPECT_CALL(scheduler, statusUpdate(driver, _))
  .WillRepeatedly([=, &finished](
  const SchedulerDriver&, const TaskStatus& status) {
CHECK_EQ(task.task_id(), status.task_id());
if (status.state() == TASK_FINISHED) {
  finished.set(Nothing());
}
  });

AWAIT_READY_FOR(finished.future(), Seconds(10));

// 

But since we don't have C++11 lambdas as gmock actions yet, instead create 
your own action:

ACTION_TEMPLATE(FinishedStatusUpdate, promise)
{
  const TaskStatus& status = arg1;
  // TODO: check
  if (status.state() == TASK_FINISHED) {
promise->set(Nothing());
  }
}

And finally, let's pull this out of s/runTask/launchTask/ so it has 
consistent behavior with launchTasks.



src/tests/fetcher_cache_tests.cpp


InvokeLambda(args, lambda)
{
  return lambda(get<0>(args), get<1>)(args), get<2>(args), ...);
}

InvokeVoidLambda(args, lambda)
{
  lambda(get<0>(args), get<1>)(args), get<2>(args), ...);
}



src/tests/fetcher_cache_tests.cpp


Let's rename this to something that is more indicative of what's happening 
when we reach or block on a FetcherProcess::_fetch.



src/tests/fetcher_cache_tests.cpp


CHECK_READY

Here and everywhere else please. ;-)



src/tests/fetcher_cache_tests.cpp


const&



src/tests/fetcher_cache_tests.cpp


Capitlization please. Everywhere. ;-)



src/tests/fetcher_cache_tests.cpp


if (strings::contains(event.request->path, COMMAND_NAME)) {

}

Below please too!



src/tests/fetcher_cache_tests.cpp


Unroll please, the switch is harder to grok below.



src/tests/fetcher_cache_tests.cpp


Path
{
  Try executable() const;
};

Path(runDirectory, commandFilename).executable();



src/tests/fetcher_tests.cpp


EXPECT_ERROR

(Search for 'isError())' and replace. ;-) )



src/tests/fetcher_tests.cpp


s//bernd/



src/tests/fetcher_tests.cpp


EXPECT_TRUE(strings::contains(fetch.failure(), "chown"));

(How about we just search and replace .find?)



src/tests/mesos.cpp


Let's double check whether or not this is really necessary?


- Benjamin Hindman


On May 1, 2015, 2:40 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated May 1, 2015, 2:40 a.m.)
> 
> 
> Review request for mesos, Ad

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On April 29, 2015, 1:46 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, lines 386-387
> > 
> >
> > How about still calling this s/reference/get/ and then checking the 
> > references? Also, we're deprecating and removing the return value of const 
> > &. Thus:
> > 
> > Option>& entry =
> >   cache.get(commandUser, uri.value());
> > 
> > if (entry.isSome()) {
> >   CHECK(entry.get()->references() > 0);
> >   ... 
> > }

After talking about this we concluded to go with explicit referencing outside 
the accessor.


> On April 29, 2015, 1:46 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, lines 394-395
> > 
> >
> > Let's get a CHECK here too:
> > 
> > CHECK(newEntry->references() > 0);

See above.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review78530
---


On April 30, 2015, 7:40 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 30, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fe

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On March 10, 2015, 8:35 a.m., Benjamin Hindman wrote:
> > src/launcher/fetcher.cpp, lines 364-366
> > 
> >
> > Why are these not CHECKs? Since you're the one setting up the 
> > FetcherInfo it seems like you should know explicitly whether or not the 
> > cache_filename was set!
> > 
> > Same for the cache_directory below as well.
> 
> Bernd Mathiske wrote:
> What if somebody else uses mesos-fetcher?

Adding a comment why this is not a check.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review75754
---


On April 30, 2015, 7:40 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 30, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of those. In dependency order:
> 
> 30033: Removes the fetcher env tests since these won't be needed any more 
> when the fetcher uses JSON in a single env var as a parameter. They never 
> tested anything that won't be covered by other tests anyway.
> 
> 30034: Makes the code structure of all fetcher tests the same. Instead of 
> calling the run method of the fetcher directly, calling through fetch(). Also 
> removes all uses of I/O redirection, which is not rea

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated April 30, 2015, 7:40 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Fixed most issues, but not all. Let's talk about theremaining ones before 
proceeding!


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp 
f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp 
c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. Introduces ab

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

> On April 29, 2015, 3:41 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, line 386
> > 
> >
> > Kill the const &.

Killed the &. Any reason this should not be const?


> On April 29, 2015, 3:41 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/fetcher.cpp, line 597
> > 
> >
> > Let's add some helper functions on Fetcher::Cache so that we can just 
> > get this information directly in the tests rather than this "helper" 
> > function.
> > 
> > // Return the cache.
> > Try> FetcherProcess::cacheFiles();
> > 
> > // Returns the number of entries in the cache.
> > size_t FetcherProcess::cacheSize();

Can we please postpone this until after refactoring fetcher injection into 
slave/containerizer? It will be much easier to make these member functions 
then. I'll put a TODO for now.


- Bernd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82017
---


On April 29, 2015, 1:42 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 29, 2015, 1:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and coverage of the new features.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
>   docs/fetcher-cache-internals.md PRE-CREATION 
>   docs/fetcher.md PRE-CREATION 
>   include/mesos/fetcher/fetcher.proto 
> 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
>   include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
>   include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
>   src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
>   src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
>   src/slave/containerizer/fetcher.hpp 
> 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 
> 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 5e5f13ed8a71ff9510b40b6032d00fd16d312622 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f2587280dc0e1d566d2b856a80358c7b3896c603 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
>   src/tests/docker_containerizer_tests.cpp 
> c9d66b3fbc7d081f36c26781573dca50de823c44 
>   src/tests/fetcher_cache_tests.cpp PRE-CREATION 
>   src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
>   src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
>   src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 
> 
> Diff: https://reviews.apache.org/r/30774/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> --- longer Description: ---
> 
> -Replaces all other reviews for the fetcher cache except those related to 
> stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
> 30618, 30621, 30626. See descriptions of tho

Re: Review Request 30774: Fetcher Cache

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review82017
---



src/slave/containerizer/fetcher.hpp


const Bytes&

?



src/slave/containerizer/fetcher.hpp


No longer used function.



src/slave/containerizer/fetcher.cpp


Please replace tab with spaces.



src/slave/containerizer/fetcher.cpp


Kill the const &.



src/slave/containerizer/fetcher.cpp


Let's be explicit for now with references:

Option> entry = cache.get();

if (entry.isSome()) {
  entry.get()->reference();
  entries[uri] = entry.get()->completion()
.then(defer(self(), [=]() { return entry.get(); });
} else {
  shared_ptr newEntry =
cache.create(cacheDirectory, commandUser, uri);

  newEntry->reference();

  entries[uri] = async(&fetchSize, uri.value(), flags.frameworks_home)
.then(defer(self(), 
}



src/slave/containerizer/fetcher.cpp


+2 not +4 here.



src/slave/containerizer/fetcher.cpp


// Wait for the URI to be downloaded into the cache (or fail).



src/slave/containerizer/fetcher.cpp


.then(defer(self(), [=]() { return entry.get(); }))

Then please kill the 'value' function above!



src/slave/containerizer/fetcher.cpp


// NOTE: We break this into two pieces because we want to be able to 
__block__ an instance of ...
return _fetch(...);



src/slave/containerizer/fetcher.cpp


For the future:

auto futures = filter(entries, [](const auto& entry) { return 
entry.isSome() ? entry.get() : None(); });

;-)



src/slave/containerizer/fetcher.cpp


Let's add some helper functions on Fetcher::Cache so that we can just get 
this information directly in the tests rather than this "helper" function.

// Return the cache.
Try> FetcherProcess::cacheFiles();

// Returns the number of entries in the cache.
size_t FetcherProcess::cacheSize();



src/slave/containerizer/fetcher.cpp


CHECK_READY



src/slave/containerizer/fetcher.cpp


Lambda-ify! ;-)



src/slave/containerizer/fetcher.cpp


Please replaces tabs with spaces.



src/slave/containerizer/fetcher.cpp


Lambda-ify! ;-)



src/slave/containerizer/fetcher.cpp


{ on newline please.



src/slave/containerizer/fetcher.cpp


CHECK_SOME



src/slave/slave.cpp


Ideally we can inject the Fetcher instance into the Slave so that we don't 
have this global recover operation that is actually per slave:

--

Fetcher fetcher(flags);

Slave slave(..., &fetcher);

Slave::registered(...)
{
  ...;
  Try recover = fetcher->recover(slaveid);
  if (recover.is...) {
...;
  }
  ...;
}

--

But for now, let's just s/recoverCache/recover/ since the fact that the 
fetcher has a cache is an implementation detail.


- Benjamin Hindman


On April 29, 2015, 8:42 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 29, 2015, 8:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic M

Re: Review Request 30774: Fetcher Cache

[Benjamin Hindman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/#review78530
---



include/mesos/type_utils.hpp


Let's also 'hash_combine' the 'extract' and 'executable' information here 
too.



src/slave/containerizer/fetcher.hpp


Great TODO, let's point the comments in slave.cpp to this TODO please!



src/slave/containerizer/fetcher.hpp


This function no longer exists!



src/slave/containerizer/fetcher.hpp


+4 please!



src/slave/containerizer/fetcher.cpp


Not used!



src/slave/containerizer/fetcher.cpp


Two spaces between top-level definitions please!



src/slave/containerizer/fetcher.cpp


Please comment/TODO why this is being used and that you plan to remove this 
once we have C++11 lambdas!



src/slave/containerizer/fetcher.cpp


How about still calling this s/reference/get/ and then checking the 
references? Also, we're deprecating and removing the return value of const &. 
Thus:

Option>& entry =
  cache.get(commandUser, uri.value());

if (entry.isSome()) {
  CHECK(entry.get()->references() > 0);
  ... 
}



src/slave/containerizer/fetcher.cpp


+2 not +4



src/slave/containerizer/fetcher.cpp


Let's get a CHECK here too:

CHECK(newEntry->references() > 0);



src/slave/containerizer/fetcher.cpp


Let's move this comment to the top of this function so that readers of the 
code have a better idea of what we're trying to accomplish sooner!



src/slave/containerizer/fetcher.cpp


Let's add another comment to remind readers why if the entry's 'completion' 
is still pending this implies that we need to DOWNLOAD_AND_CACHE? For example:

// Since the entry is not yet "complete", i.e., 'completion().isPending()', 
// then it must be the case that we created the entry in
// FetcherProcess::fetch otherwise the entry should have been
// in the cache and we would have waited for the completion in
// FetcherProcess::fetch.



src/slave/slave.cpp


s/recoverCache/recover/ <-- Since in the future there might be other 
"recovery" things that need to get done that doesn't have anything to do with 
the cache.

Also, let's please leave a comment here that explains why we're calling a 
static method rather than invoking a method on an instance of a 'Fetcher' 
directly. Our intuition is that this will likely have to change in the future.



src/slave/slave.cpp


Comment here too, please, thanks!


- Benjamin Hindman


On April 29, 2015, 8:42 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30774/
> ---
> 
> (Updated April 29, 2015, 8:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and 
> MESOS-2074
> https://issues.apache.org/jira/browse/MESOS-2057
> https://issues.apache.org/jira/browse/MESOS-2069
> https://issues.apache.org/jira/browse/MESOS-2070
> https://issues.apache.org/jira/browse/MESOS-2072
> https://issues.apache.org/jira/browse/MESOS-2073
> https://issues.apache.org/jira/browse/MESOS-2074
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Almost all of the functionality in epic MESOS-336. Downloaded files from 
> CommandInfo::URIs can now be cached in a cache directory designated by a 
> slave flag. This only happens when asked for by an extra flag in the URI and 
> is thus backwards-compatible. The cache has a size limit also given by a new 
> slave flag. Cache-resident files are evicted as necessary to make space for 
> newly fetched ones. Concurrent attempts to cache the same URI leads to only 
> one download. The fetcher program remains external for safety reasons, but is 
> now augmented with more elaborate parameters packed into a JSON object to 
> implement specific fetch actions for all of the above. Additional testing 
> includes fetching from (mock) HDFS and co

Re: Review Request 30774: Fetcher Cache

[Bernd Mathiske]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30774/
---

(Updated April 29, 2015, 1:42 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Rebased.


Bugs: MESOS-2057, MESOS-2069, MESOS-2070, MESOS-2072, MESOS-2073, and MESOS-2074
https://issues.apache.org/jira/browse/MESOS-2057
https://issues.apache.org/jira/browse/MESOS-2069
https://issues.apache.org/jira/browse/MESOS-2070
https://issues.apache.org/jira/browse/MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2073
https://issues.apache.org/jira/browse/MESOS-2074


Repository: mesos


Description
---

Almost all of the functionality in epic MESOS-336. Downloaded files from 
CommandInfo::URIs can now be cached in a cache directory designated by a slave 
flag. This only happens when asked for by an extra flag in the URI and is thus 
backwards-compatible. The cache has a size limit also given by a new slave 
flag. Cache-resident files are evicted as necessary to make space for newly 
fetched ones. Concurrent attempts to cache the same URI leads to only one 
download. The fetcher program remains external for safety reasons, but is now 
augmented with more elaborate parameters packed into a JSON object to implement 
specific fetch actions for all of the above. Additional testing includes 
fetching from (mock) HDFS and coverage of the new features.


Diffs (updated)
-

  docs/configuration.md 54c4e31ed6dfed3c23d492c19a301ce119a0519b 
  docs/fetcher-cache-internals.md PRE-CREATION 
  docs/fetcher.md PRE-CREATION 
  include/mesos/fetcher/fetcher.proto 311af9aebc6a85dadba9dbeffcf7036b70896bcc 
  include/mesos/mesos.proto 967b1e3bbfb3f6b71d5a15d02cba7ed5ec21816f 
  include/mesos/type_utils.hpp 044637481e5405d4d6f61653a9f9386edd191deb 
  src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
  src/hdfs/hdfs.hpp 968545d9af896f3e72e156484cc58135405cef6b 
  src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 
  src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
  src/slave/containerizer/docker.hpp b25ec55bf3cd30d6e8a804d09d90c632a7d12e3f 
  src/slave/containerizer/docker.cpp f9fc89ad7e3c853c3f9f6dcf9aa68e54dc1888c6 
  src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
  src/slave/containerizer/fetcher.cpp 9e9e9d0eb6b0801d53dec3baea32a4cd4acdd5e2 
  src/slave/containerizer/mesos/containerizer.hpp 
5e5f13ed8a71ff9510b40b6032d00fd16d312622 
  src/slave/containerizer/mesos/containerizer.cpp 
f2587280dc0e1d566d2b856a80358c7b3896c603 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
  src/slave/slave.cpp c78ee3c9e7fc38ad364e83f4abe267e86bfbbc13 
  src/tests/docker_containerizer_tests.cpp 
c9d66b3fbc7d081f36c26781573dca50de823c44 
  src/tests/fetcher_cache_tests.cpp PRE-CREATION 
  src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e 
  src/tests/mesos.hpp 19db71217f0a3f1ab17a6fd4408f8251410d731d 
  src/tests/mesos.cpp bc082e8d91deb2c5dd64bbc3f0a8a50fa7d19264 

Diff: https://reviews.apache.org/r/30774/diff/


Testing
---

make check

--- longer Description: ---

-Replaces all other reviews for the fetcher cache except those related to 
stout: 30006, 30033, 30034, 30036, 30037, 30039, 30124, 30173, 30614, 30616, 
30618, 30621, 30626. See descriptions of those. In dependency order:

30033: Removes the fetcher env tests since these won't be needed any more when 
the fetcher uses JSON in a single env var as a parameter. They never tested 
anything that won't be covered by other tests anyway.

30034: Makes the code structure of all fetcher tests the same. Instead of 
calling the run method of the fetcher directly, calling through fetch(). Also 
removes all uses of I/O redirection, which is not really needed for debugging, 
and thus the next patch can refactor fetch() and run(). (The latter comes in 
two varieties, which complicates matters without much benefit.)

30036: Extends the CommandInfo::URI protobuf with a boolean "caching" field 
that will later cause fetcher cache actions. Also introduces the notion of a 
cache directory to the fetcher info protobuf. And then propagates these 
additions throughout the rest of the code base where applicable. This includes 
passing the slave ID all the way down to the place where the cache dir name is 
constructed.

30037: Extends the fetcher info protobuf with "actions" (fetch directly 
bypassing the cache, fetch through the cache, retrieve from the cache). 
Switches the basis for dealing with uris to "items", which contain the uri, the 
action, and potentially a cache file name. Refactors fetch() and run(), so 
there is only one of each. Introduces about half of the actual cache logic, 
including a hashmap of cache file objects