mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 30037: Introduced fetcher cache actions
Date Tue, 20 Jan 2015 15:13:31 GMT


> On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote:
> > include/mesos/fetcher/fetcher.proto, line 38
> > <https://reviews.apache.org/r/30037/diff/1/?file=825117#file825117line38>
> >
> >     These names seem to change everytime I lookat this.
> >     
> >     FETCH_DIRECTLY sounds exactly the same meaning as just FETCH though?
> >     
> >     Since there is FETCH_AND_CACHE already, shouldn't FETCH convey we're not caching?

You are right. Either "fetch" is anything and everything or it is only the downloading part
and I mixed this up here. I intended "fetch" to be whatever gets something into the work dir,
no matter which path is taken. How about DOWNLOAD_DIRECTLY, DOWNLOAD_AND_CACHE, RETRIEVE_FROM_CACHE
and "fetching" is any of the above? That's what I am going to use for now. Suggestions remaining
welcome.


> On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote:
> > include/mesos/fetcher/fetcher.proto, line 54
> > <https://reviews.apache.org/r/30037/diff/1/?file=825117#file825117line54>
> >
> >     optional because we don't always cache right?
> >     probably worth commenting.

Good idea to add a comment that desribes this. Will do that.


> On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote:
> > src/slave/containerizer/fetcher.cpp, line 169
> > <https://reviews.apache.org/r/30037/diff/1/?file=825120#file825120line169>
> >
> >     I wonder if we can refactor this 3 level if else.
> >     
> >     It's a bad smell looking at this since if we add another action it becomes another
nesting downwards.

Let's wait with this refactoring until we have the final version in MESOS-2057! This code
stretch will change for sure until then. Then indeed, this must be well-factored. I will make
a note not to forget it.


> On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote:
> > src/slave/containerizer/fetcher.cpp, line 177
> > <https://reviews.apache.org/r/30037/diff/1/?file=825120#file825120line177>
> >
> >     how do we know file.get().get() will succeed?
> >     we should at least do CHECK_SOME(file.get())

The first get() is guarded by a condition that tests for isNone(), the second is an owned
pointer dereferentiation.


> On Jan. 19, 2015, 10:08 a.m., Timothy Chen wrote:
> > src/slave/containerizer/fetcher.cpp, line 185
> > <https://reviews.apache.org/r/30037/diff/1/?file=825120#file825120line185>
> >
> >     Seems like the legacy behavior is that there is no caching and everyone downloads
its own copy.
> >     
> >     Now if someone any downloads the same file the same time it instead returns
a Failure (does this fail the task too?)
> >     
> >     I wonder if we should at least exhibit the same behavior if we don't support
concurrent fetching, at least we shouldn't make a task fail if somehow their tasks are donwloading
the same file at once.

I prefer not to implement any behavior here that gets deleted in the next step. That would
be wasted engineering. Let's just assume nobody is trying to use the cache at all until we
give the green light. This code here is a stepping stone in the spirit of keeping reviews
small. It is impossible to create a working cache AND keep the single review request small.


- Bernd


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


On Jan. 19, 2015, 8:05 a.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30037/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2015, 8:05 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2069
>     https://issues.apache.org/jira/browse/MESOS-2069
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Extends the fetcher info protobuf with "actions" (fetch directly, through cache, retrieve
from cache). Switches the basis for dealing with uris to "items", which contain the uri, the
action, and potentially and 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 for bookkeeping and basic operations on it. 
> 
> 
> Diffs
> -----
> 
>   include/mesos/fetcher/fetcher.proto facb87b92bf3194516f636dcc348e136af537721 
>   src/launcher/fetcher.cpp fed0105946da579a38357a30e7ae56e646e05b89 
>   src/slave/containerizer/fetcher.hpp 1db0eaf002c8d0eaf4e0391858e61e0912b35829 
>   src/slave/containerizer/fetcher.cpp 5993670f7899233efa1e6acef4b0c7856e32f748 
> 
> Diff: https://reviews.apache.org/r/30037/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>


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