----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45046/#review124456 -----------------------------------------------------------
Can you also add a CHANGELOG entry for this under "Additonal API changes" section for 0.29.0 (create one if it doesn't exist)? src/launcher/fetcher.cpp (lines 252 - 260) <https://reviews.apache.org/r/45046/#comment187023> The way this is written, we will throw an error if basename cannot be determined irrespective of whether filename was specified. How about: ``` Try<string> basename = uri.has_filename() ? uri.filename() : Fetcher::basename(uri.value()); ``` src/launcher/fetcher.cpp (lines 295 - 298) <https://reviews.apache.org/r/45046/#comment187024> ditto. see above. src/launcher/fetcher.cpp (lines 295 - 298) <https://reviews.apache.org/r/45046/#comment187025> ditto. see above. src/slave/containerizer/fetcher.cpp (lines 156 - 160) <https://reviews.apache.org/r/45046/#comment187026> Why are you validating the filename as an URI? IIUC, filename should be a simple relative path? src/tests/fetcher_cache_tests.cpp (lines 689 - 697) <https://reviews.apache.org/r/45046/#comment187027> //.... const string executablePath = path::join( task.get().runDirectory.value, customFilename); EXPECT_TRUE(isExecutable(executablePath)); // The script... const string outputPath = path::join( task.get().runDirectory.value, COMMAND_NAME); EXPECT_TRUE(os::exists(outputPath + taskName(index))); src/tests/fetcher_tests.cpp (lines 645 - 647) <https://reviews.apache.org/r/45046/#comment187028> 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. src/tests/fetcher_tests.cpp (line 677) <https://reviews.apache.org/r/45046/#comment187030> kill this. if you did as suggested above, this will be automatically cleanedup as part of the teardown. src/tests/fetcher_tests.cpp (lines 684 - 685) <https://reviews.apache.org/r/45046/#comment187031> ditto. see above. src/tests/fetcher_tests.cpp (line 716) <https://reviews.apache.org/r/45046/#comment187033> kill this. - Vinod Kone 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 > >
