> On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.hpp, line 98 > > <https://reviews.apache.org/r/30037/diff/8/?file=833520#file833520line98> > > > > This generates a unique pid, in case of multiple simultaneous fetchers?
Each slave has its own fetcher, so we need unique fetcher pids. (You could also get subtle bugs in recovery tests otherwise.) > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > include/mesos/fetcher/fetcher.proto, line 49 > > <https://reviews.apache.org/r/30037/diff/8/?file=833518#file833518line49> > > > > CacheAction? Nope. At leat one of these actions does not involve the cache. These are fetch actions. But "fetch" is redundant. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.hpp, lines 131-134 > > <https://reviews.apache.org/r/30037/diff/8/?file=833520#file833520line131> > > > > Is the custom output redirection still supported, or was it never used? > > Can we be sure that Twitter or another user isn't using the redirection for > > their own custom fetcher? This is log output that is highly unlikely for anything but human consumption when debugging. And then it is just as easy to look at the sandbox, where it goes without redirection. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.hpp, line 166 > > <https://reviews.apache.org/r/30037/diff/8/?file=833520#file833520line166> > > > > Why does FetcherProcess need to be a friend class? Seems like it > > shouldn't be necessary. ../../src/slave/containerizer/fetcher.cpp:414:53: error: 'promise' is a private member of 'mesos::internal::slave::FetcherProcess::CacheFile' Promise<string>& promise = cacheFile.get().get()->promise; ^ ../../src/slave/containerizer/fetcher.hpp:142:35: note: declared private here process::Promise<std::string> promise; field "name" is public, because it is const. "promise" is not public. It could be, but I'd rather hide it. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, line 58 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line58> > > > > I'm assuming this gets used for something other than extension() in a > > future patch, right? correct. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 68-69 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line68> > > > > What about fetching from ftp://, ftps, hdfs, hftp, s3, s3n, etc? This > > "Malformed URI" logic used to apply to ftp/ftps as well, but I'm guessing > > s3 and hdfs also wouldn't like fetching from a URI that doesn't have a '/', > > or ends in '/'. Not sure why the protocol string really matters when > > determining a basename for the cache file. I do not know either. I just carried this code over from launcher/fetcher.cpp, and I would like to see a clean, comprehensive solution in stout. Hence the TODO. My new code should do exactly what the code before it did in this regard. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 153-156 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line153> > > > > Where does the user validation happen? Wouldn't want to create a > > directory with a username that isn't a valid directory_name. Wouldn't want > > to download anything if we didn't have permissions for that user anyway. Right. Tim spotted this, too. I will now bring this validation back, which I had scheduled to reintroduce in too late a review. But spome part of it will be refactored to somewhere else later... > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, line 224 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line224> > > > > Could this ever error? No, but one of the next patches will make it potentially error. See TODO just below. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 468-469 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line468> > > > > Which filesystems don't accept files starting with a digit? Can you > > give an example in the comment? I am getting old :-) OK, there is an alternate reason, which I will state now: // We put a prefix before the serial number so we can later easily // find cache files with os::find() to support testing. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 478-480 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line478> > > > > So, cache_filename doesn't include the original basename at all? Just > > `c1234.tgz`? That makes it hard for an admin to debug something going wrong > > with the fetcher/cache. Do we at least log (even VLOG) somewhere that URL > > "foo" is being downloaded to the cache as "c1234.bar"? Maybe when we > > allocateCachefile Every download needs a unique filename. Options: - wrap the download in a dir => do we run out of dir entries in the fs? - concatenate the original basename => who knows how long that might be? Combined too long? Might even run into path length limits. - add part of the basename just for humans => I'll go for that! Say, up to 20 chars? Yes, we log everything in stderr in the sandbox. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.hpp, line 193 > > <https://reviews.apache.org/r/30037/diff/8/?file=833520#file833520line193> > > > > Option vs. Try? No error when returning that no entry is present => Option. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 318-321 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line318> > > > > What if somebody still wants to set custom FDs for out/err? See above. Unlikely. > On Jan. 28, 2015, 2:47 a.m., Adam B wrote: > > src/slave/containerizer/fetcher.cpp, lines 385-388 > > <https://reviews.apache.org/r/30037/diff/8/?file=833521#file833521line385> > > > > I can't tell if these are lined up correctly, but they look off in RB. Fixed. - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30037/#review69973 ----------------------------------------------------------- On Jan. 28, 2015, 12:54 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30037/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2015, 12:54 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 > >
