> 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.
>
> Bernd Mathiske wrote:
> 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.
>
> Adam B wrote:
> Not "exactly" what we were doing in 0.21.1. See
> https://github.com/apache/mesos/blob/0.21.1/src/launcher/fetcher.cpp#L114
> As I mentioned, this check also used to include ftp/ftps uris. Also, it
> used to `LOG(ERROR) << "Malformed URL (missing path)";`
>
> You should update the comment to clarify what the previous bug/feature
> intended. Here's my interpretation: 1) Check that there is at least one '/'
> in the URI after the '://', since a fetch of "http://mesos.apache.org"; is not
> a valid fetcher URI, it's just a website/hostname; 2) Check that the URI does
> not end in a '/', since that's just a website/directory like
> "http://mesos.apache.org/downloads/";. Granted, these URIs could just pull
> down an index.html, but that's implicit behavior, and perhaps it's better to
> explicitly fetch "http://mesos.apache.org/index.html"; if that's what you want.
> I agree that we could do more intelligent URI parsing/verification, but
> maybe we should just let hdfs or net::download check for those errors. I'm
> not even sure why we originally added these checks. I traced them back to
> 2012; you'll have to ask Flo:
> commit ecf3b4a14d87c0eab2543f0fbc6c6efe208abf9c
> Author: Benjamin Hindman <[email protected]>
> Date: Thu Mar 15 23:22:43 2012 +0000
> Adding support to retrieve files (uri's) via HTTP/FTP (contributed by
> Florian Leibert).
Oh yeah, we need a '/' in the "path", and not as its last character, since we
want `path.substr(path.find_last_of("/") + 1)` to return a non-empty string for
basename(). Any other "invalid" characters will just pass through.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30037/#review69973
-----------------------------------------------------------
On Jan. 28, 2015, 9:33 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, 9:33 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
>
>
> 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
>
>
