> On April 19, 2016, 4:08 p.m., Jiang Yan Xu wrote: > > src/launcher/fetcher.cpp, line 252 > > <https://reviews.apache.org/r/46168/diff/1/?file=1343101#file1343101line252> > > > > `basename` usually specifically refers to the `the component following > > the final '/'`. > > > > So here s/basename/outputFile/
It is a proper basename in the case where we're just truncating the URI, but I agree that outputFile is more general given the expanded use here. > On April 19, 2016, 4:08 p.m., Jiang Yan Xu wrote: > > docs/fetcher.md, line 89 > > <https://reviews.apache.org/r/46168/diff/1/?file=1343100#file1343100line89> > > > > I feel that "filename" is a bit odd when it can be a path. Many > > utilities simply call this a file, or output file to be more generic and > > flexible (when they do take a path). > > > > e.g., > > ``` > > wget --output-document file > > url --output file > > tar --file file > > gcc -o outfile > > clang -o output-file > > ``` > > > > I think `output_file` sounds good. (Notice the snake_casing in proto). > > What do you think? > > > > We need to change docs and code (including CHANGELOG) elsewhere too but > > luckily the API is not released yet. > > > > In CHANGELOG we can still group things under [MESOS-4735], just > > s/'filename'/'output_file'/. output_file sounds good to me. > On April 19, 2016, 4:08 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/fetcher.cpp, lines 150-153 > > <https://reviews.apache.org/r/46168/diff/1/?file=1343102#file1343102line150> > > > > What does this check against? > > > > Path(filename).basename() never returns an Error(). Ah, got it confused with Fetcher::basename, which can return an Error. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46168/#review129487 ----------------------------------------------------------- On April 14, 2016, 12:06 a.m., Michael Browning wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46168/ > ----------------------------------------------------------- > > (Updated April 14, 2016, 12:06 a.m.) > > > Review request for mesos, Bernd Mathiske, Jiang Yan Xu, and Zhitao Li. > > > Bugs: MESOS-5119 > https://issues.apache.org/jira/browse/MESOS-5119 > > > Repository: mesos > > > Description > ------- > > Add subdirectory support to URI.filename field. > > URI.filename allows the user to specify the name of the file that will > be saved in the sandbox when the URI is fetched, but previously it would > fail at fetch time if "filename" had a directory component. This change > allows users to specify a relative path for custom filenames within the > sandbox. > > > Diffs > ----- > > docs/fetcher.md fd6d8a78bd35c5644dceff7005dd7dfd9f5f2171 > src/launcher/fetcher.cpp 47583eeaed53b3e7ed4db26fee7cdd2fae5e0c9d > src/slave/containerizer/fetcher.cpp > d5910ad570371ba54580be5bb94344a1de38d1f9 > src/tests/fetcher_cache_tests.cpp 9ffcd2375f1203bd3d7c5d0cc898e955d5cb124e > src/tests/fetcher_tests.cpp 23a8dc5f4402c5613744753284aabbe3d09bf797 > > Diff: https://reviews.apache.org/r/46168/diff/ > > > Testing > ------- > > Three tests were added. In fetcher_tests.cpp, CustomFilenameSubdirectory > tests that the fetcher creates a file in a specified subdirectory in the > sandbox, and AbsoluteCustomSubdirectoryFails tests that a custom filename > with an absolute path is rejected. In fetcher_cache_tests.cpp, > CachedCustomFilenameWithSubdirectory tests that the same behavior holds when > the URI is fetched from the cache. > > > Thanks, > > Michael Browning > >
