> On March 20, 2016, 6:36 p.m., Vinod Kone wrote: > > src/slave/containerizer/fetcher.cpp, lines 156-160 > > <https://reviews.apache.org/r/45046/diff/3/?file=1307410#file1307410line156> > > > > Why are you validating the filename as an URI? IIUC, filename should be > > a simple relative path?
In the absence of a URI schema, validateUri basically validates exactly that case -- but you're right, that's not really in line with the semantics of the validateUri method. I'll factor that logic out into a separate validateFilename function. > On March 20, 2016, 6:36 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, lines 645-647 > > <https://reviews.apache.org/r/45046/diff/3/?file=1307412#file1307412line645> > > > > I know you followed the existing pattern in this file but we shouldn't > > do mktemp() like this in the tests because if the test fails for whatever > > reason the temp file and the directory are leaked. > > > > The FetcherTest fixture inherits from TemporaryDirectoryTest so that > > the test runs in a temporary sandbox. > > > > So you should do: > > > > ``` > > Try<string> dir = os::mkdtemp("./XXXX")); > > ASSERT_SOME(dir); > > > > Try<string> path = os::mktemp(path::join(dir.get(), "XXXX")); > > ASSERT_SOME(path); > > ``` > > > > Feel free to fix the other tests in this file in a subsequent review or > > just add a TODO for now. Happy to fix the other tests in this file. I think it'd be simpler to do it in a separate review -- should I also create an issue for that? - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45046/#review124456 ----------------------------------------------------------- On March 19, 2016, 10:03 p.m., Michael Browning wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45046/ > ----------------------------------------------------------- > > (Updated March 19, 2016, 10:03 p.m.) > > > Review request for mesos, Vinod Kone and Zhitao Li. > > > Bugs: MESOS-4735 > https://issues.apache.org/jira/browse/MESOS-4735 > > > Repository: mesos > > > Description > ------- > > Created URI.filename to name fetched files in sandbox where appropriate. > > > Diffs > ----- > > docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a > include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b > include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b > src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 > src/slave/containerizer/fetcher.cpp > 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 > src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 > src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e > > Diff: https://reviews.apache.org/r/45046/diff/ > > > Testing > ------- > > There are two paths by which a file gets fetched to the executor sandbox: the > without-cache path, where the fetcher downloads the file directly from the > specified URI, and the with-cache path, where it copies it from the cache. In > both cases, we verify that the file is saved to the sandbox directory with > the name specified by the "filename" field in the CommandInfo.URI proto. > > > Thanks, > > Michael Browning > >
