> On Jan. 23, 2015, 5:01 p.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, lines 37-38 > > <https://reviews.apache.org/r/30037/diff/6/?file=829684#file829684line37> > > > > Maybe it would be better to call this BYPASS_CACHE, and then the other > > two could be combined into USE_CACHE. At that point it almost sounds like a > > boolean. Is there a discussion somewhere about what this variable should > > express?
The other two cannot be combined. mesos-fetcher needs to know whether to grab something from the cache or whetehr to place it there, first. It does not make its own decisions about that! FetcherProcess in the slave does. There is no JIRA for this, this is all regarded as implementation detail so far. > On Jan. 23, 2015, 5:01 p.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, lines 44-45 > > <https://reviews.apache.org/r/30037/diff/6/?file=829684#file829684line44> > > > > How does this fail for an item not present in the cache? Why wouldn't > > you just want to check for presence in the cache, retrieve if present and > > download&cache if absent? Because I already know what's in the cache and what isn't. This bookkeeping is NOT in mesos-fetcher, to which this is a parameter. It is in FetcherProcess, which is in the slave. > On Jan. 23, 2015, 5:01 p.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, line 49 > > <https://reviews.apache.org/r/30037/diff/6/?file=829684#file829684line49> > > > > Why make this required, instead of optional with a default action? I like the more explicit way better. Otherwise you'd have to explain what it means if something is not there. You could also go for "present", and for "not present" use a boolean to distinguish the other two cases. My take is that the program is easier to read, with less comments, when you have expressive enums. And for users, there is more explicit info in the JSON that gets transferred. > On Jan. 23, 2015, 5:01 p.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, line 50 > > <https://reviews.apache.org/r/30037/diff/6/?file=829684#file829684line50> > > > > You could probably get away with `cache_filename` instead Fixing this. > On Jan. 23, 2015, 5:01 p.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, line 55 > > <https://reviews.apache.org/r/30037/diff/6/?file=829684#file829684line55> > > > > Why introduce this in the previous patch if you're just going to > > move/change it in this one? We should either do the protobuf changes in > > backwards-compatible iterations, or all in one patch, once we know the > > desired format. I wanted to keep the individual patches more self-sufficient, as well as compiling and running. The latter so that they can be committed individually. > On Jan. 23, 2015, 5:01 p.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, line 59 > > <https://reviews.apache.org/r/30037/diff/6/?file=829684#file829684line59> > > > > New fields must take new/unused field ids, so this should be `repeated > > Item items = 7;` No requirement to be compatible here. > On Jan. 23, 2015, 5:01 p.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, line 62 > > <https://reviews.apache.org/r/30037/diff/6/?file=829684#file829684line62> > > > > Where did this go? Turns out the env var HADOOP_HOME from which this was a carry-over is not used anywhere in mesos-fetcher. It has been passed as an argument for ages and not used at all. I checked back to April 2014. Nothing. So I removed it. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30037/#review69514 ----------------------------------------------------------- On Jan. 22, 2015, 1:07 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30037/ > ----------------------------------------------------------- > > (Updated Jan. 22, 2015, 1:07 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 > d290f95251def3952c5ee34f600e1d71467f6293 > > Diff: https://reviews.apache.org/r/30037/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Bernd Mathiske > >
