-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27606/#review62386
-----------------------------------------------------------
Thanks for taking this on Cody! This looks like a decent hack, just some minor
cleanup and documentation of the issues with the existing code, and we can get
this committed!
A few comments on the overall code, these are not your fault, was just noticing
these while reading the code:
(1) With the path "standardization", issuing a read of /log/ would return data
from the /log file? Seems wrong to me.
(2) Currently, if one attaches a file as a directory, e.g. attach(".../log",
"/log/") this will work? We should validate against that.
(3) Looks like browse.json is missing a BadRequest when one tries to browse a
file instead of a directory (or symlink to a directory).
Feel free to punt on all of these for now by dropping some TODOs in the code,
we could add some `DISABLED_` tests or leave TODOs in the existing tests to
show the issues.
src/files/files.hpp
<https://reviews.apache.org/r/27606/#comment104441>
Let's document what this returns:
// Returns whether the path attached under 'name' was detached successfully.
Might as well document when 'attach' can fail, but if you decide to, please
do it in a separate patch. :)
src/files/files.cpp
<https://reviews.apache.org/r/27606/#comment104443>
A few reminders:
(1) Please use two spaces between top level definitions here.
(2) Please remember to place the & next to the type.
(3) You don't need the std:: qualifiers here since there's a using clause
already.
Take your time! I'd recommend doing self-review of your patches before you
publish, it requires some up-front time, but it results in patches landing much
faster because reviews are more straightforward!
I see reviewers often getting distracted with low level issues, reducing
the reviewer's focus on high level design and overall code quality. Look, I'm
doing it right now! ;) :D
src/files/files.cpp
<https://reviews.apache.org/r/27606/#comment104454>
Ok, for now, how about we call this 'sanitize' and have it also perform
flattening of slashes (per the [unix
spec](http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266)).
Let's document it:
// Returns a sanitized version of the path, which currently includes:
// (1) Flattening slashes (// -> /).
// (2) Stripping trailing slashes.
src/files/files.cpp
<https://reviews.apache.org/r/27606/#comment104455>
Per my top level comment, could you add a TODO to ingore files mounted as
directories?
E.g. attach("/log", "log/")
src/tests/files_tests.cpp
<https://reviews.apache.org/r/27606/#comment104457>
What is this? Why should it work?
src/tests/files_tests.cpp
<https://reviews.apache.org/r/27606/#comment104458>
This should work per my top level comment ;)
- Ben Mahler
On Nov. 20, 2014, 12:34 a.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27606/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 12:34 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: mesos-1877
> https://issues.apache.org/jira/browse/mesos-1877
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Files attach, detach, and browse now all call a single function to
> standardize the path. Before attach, browse did the normalization. Detach did
> not.
>
> Switch to strings::tokenize() for splitting apart the path into tokens, as
> that is more canonical across the codebase, used by libprocess for
> proccessing paths.
>
> Update detach to return a bool of whether or not the detach does anything
> which means that it can be tested.
>
> Add some additional testing to catch the inconsistencies in path handlign
> between attach and detach previously, some of the oddness in current path
> handling so that they can be explicitly seen when changes happen.
>
>
> Diffs
> -----
>
> src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
> src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
> src/tests/files_tests.cpp 9ad6db51873b96b3cd759523cef9748f6823fb7e
>
> Diff: https://reviews.apache.org/r/27606/diff/
>
>
> Testing
> -------
>
> make distcheck on ubuntu 14.04
>
>
> Thanks,
>
> Cody Maloney
>
>
