mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 46168: Add subdirectory support to URI.filename field.
Date Tue, 19 Apr 2016 16:08:36 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46168/#review129487
-----------------------------------------------------------




docs/fetcher.md (line 89)
<https://reviews.apache.org/r/46168/#comment192972>

    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'/.



src/launcher/fetcher.cpp (line 252)
<https://reviews.apache.org/r/46168/#comment192927>

    `basename` usually specifically refers to the `the component following the final '/'`.
    
    So here s/basename/outputFile/



src/launcher/fetcher.cpp (lines 253 - 268)
<https://reviews.apache.org/r/46168/#comment192969>

    To avoid setting a Try with a "" when Try explicity doesn't have a default contructor,
here we can keep the conditional operator but separately mkdir if needed.
    
    ```
    // Prepare output directory if necessary.
    if (uri.has_filename()) {
      string dirname = Path(uri.filename()).dirname();
      if (dirname != ".") {
        Try<Nothing> result =
          os::mkdir(path::join(sandboxDirectory, uriDirname), true);
    
        if (result.isError()) {
          return Error("Unable to create subdirectory in sandbox");
        }
      }
    }
    
    Try<string> outputFile = uri.has_filename()
      ? uri.filename()
      : Fetcher::basename(uri.value());
    ```



src/launcher/fetcher.cpp (line 255)
<https://reviews.apache.org/r/46168/#comment192926>

    s/uriDirname/dirname/



src/launcher/fetcher.cpp (line 258)
<https://reviews.apache.org/r/46168/#comment192925>

    2 space indentation here.



src/launcher/fetcher.cpp (line 261)
<https://reviews.apache.org/r/46168/#comment192975>

    Here let's add the `dirname` to the error mesasge.



src/launcher/fetcher.cpp (line 308)
<https://reviews.apache.org/r/46168/#comment192977>

    Same as above.
    
    It sucks that a lot of code is duplicated here but the newly added code is only a part
of it. Let's add a TODO to "refactor common logic in fetchFromCache and fetchBypassingCache
into a helper".



src/slave/containerizer/fetcher.cpp (lines 150 - 153)
<https://reviews.apache.org/r/46168/#comment192923>

    What does this check against?
    
    Path(filename).basename() never returns an Error().



src/slave/containerizer/fetcher.cpp (lines 155 - 158)
<https://reviews.apache.org/r/46168/#comment192924>

    `filename` is not guaranteed to be at least one character right?
    
    Plus this doesn't prevent users from specifying something like `subdir/../../../../`.

    
    For the latter issue ideally there should be a method `Path::canonical` that resolves
it but it's OK if we punt on it for now. Leave a TODO here for it here.
    
    Also s/validateFilename/validateOutputFile/



src/tests/fetcher_tests.cpp (lines 105 - 106)
<https://reviews.apache.org/r/46168/#comment192978>

    Nothing wrong with this but the source doesn't have to reside in a subdir. :)
    
    We can remove this so there's no confusion.


- Jiang Yan Xu


On April 13, 2016, 5:06 p.m., Michael Browning wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46168/
> -----------------------------------------------------------
> 
> (Updated April 13, 2016, 5:06 p.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message