From reviews-return-31646-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Tue Apr 19 23:04:01 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 954BD19891 for ; Tue, 19 Apr 2016 23:04:01 +0000 (UTC) Received: (qmail 37202 invoked by uid 500); 19 Apr 2016 23:04:01 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 37174 invoked by uid 500); 19 Apr 2016 23:04:01 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 37158 invoked by uid 99); 19 Apr 2016 23:04:01 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 19 Apr 2016 23:04:01 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id D34A22B10F9; Tue, 19 Apr 2016 23:03:56 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============5072095992942122911==" MIME-Version: 1.0 Subject: Re: Review Request 46168: Add subdirectory support to URI.filename field. From: Michael Browning To: Zhitao Li , Bernd Mathiske , Jiang Yan Xu Cc: Mesos ReviewBot , Michael Browning , mesos Date: Tue, 19 Apr 2016 23:03:56 -0000 Message-ID: <20160419230356.8129.902@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Michael Browning X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/46168/ X-Sender: Michael Browning References: <20160419160836.8135.13859@reviews.apache.org> In-Reply-To: <20160419160836.8135.13859@reviews.apache.org> Reply-To: Michael Browning X-ReviewRequest-Repository: mesos --===============5072095992942122911== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On April 19, 2016, 4:08 p.m., Jiang Yan Xu wrote: > > src/launcher/fetcher.cpp, line 252 > > > > > > `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 > > > > > > 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 > > > > > > 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 > > --===============5072095992942122911==--