You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Cody Maloney <co...@mesosphere.io> on 2014/10/15 20:46:29 UTC
Review Request 26766: MESOS-1878: Add additional helper functions to
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/
-----------------------------------------------------------
Review request for mesos and Timothy Chen.
Bugs: MESOS-1878
https://issues.apache.org/jira/browse/MESOS-1878
Repository: mesos-git
Description
-------
Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
Diff: https://reviews.apache.org/r/26766/diff/
Testing
-------
make distcheck
Thanks,
Cody Maloney
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Cody Maloney <co...@mesosphere.io>.
> On Oct. 16, 2014, 4:38 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 25
> > <https://reviews.apache.org/r/26766/diff/1/?file=722432#file722432line25>
> >
> > Without starting to look at what RoundTrip does I really have no clue what it means, since it doesn't really suggest splitting the string and testing that the joining also works.
> >
> > I'm horrible with names, but I just think naming it resembles string functions it's calling is easier to understand. Don't you think something like EXPECT_SPLIT_EQ_JOIN("a", vector<string>({"a"}) is easier to understand at first glance than RoundTrip?
roundTrip is the standard terminology for a cycle to something and back. Note that the roundTripReduce explicitly isn't checking that they produce the same reuslts.
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56883
-----------------------------------------------------------
On Oct. 23, 2014, 4:52 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 4:52 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56883
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp
<https://reviews.apache.org/r/26766/#comment97321>
Without starting to look at what RoundTrip does I really have no clue what it means, since it doesn't really suggest splitting the string and testing that the joining also works.
I'm horrible with names, but I just think naming it resembles string functions it's calling is easier to understand. Don't you think something like EXPECT_SPLIT_EQ_JOIN("a", vector<string>({"a"}) is easier to understand at first glance than RoundTrip?
- Timothy Chen
On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Cody Maloney <co...@mesosphere.io>.
> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 35
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line35>
> >
> > IMO calling asAbsolute("") sounds more like a bug then just returning "/".
Practically there isn't a good place to return such an error. This code is generally acting on paths given by a user of mesos / caller of a mesos API, so doing a assert() or CHECK() definitely is not the right thing.
The given behavior is a simple, reasonable way to continue without erroring. In debugging code which does call this wrong, you get exactly the same thing every time, which doesn't make it particularly hard to track down.
> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 46
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line46>
> >
> > It's quite unclear what path::clean means, and have to read the tests to see what you're trying to do. When is this useful?
> > If we're trying to clean up user configuration, I rather we expose the error instead of trying to clean up mistakes.
I can add a little documentation about the sorts of things it cleans up.
It's not user configuration, it is runtime parameters to API calls where this comes in the most. Someone calls `/files/browse.json?path=foo` `/files/browse.json?path=foo/` `/files/browse.json?path=/foo` `/files/browse.json?path=/foo/`. All of those are reasonable ways to get to the same place in the filesystem. They follow what people expect from the filesystem, and so we need to handle them all. I can document some of the exact behavior here, but really it is the combined behavior of split and join, as those do the cleanup implicitly in their operation.
All the normalization of the paths used to live just in files/files.cpp and was applied inconsistenly (even within the one file). The path::clean() makes one consistent place for cleaning up a path (which may be virtual, so we can't use the OS calls) to live.
> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 127
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line127>
> >
> > Illegal based on?
It doesn't make any logical sense. You can't have an empty directory name inside a directory. Joining it would result in a/ which isn't what is desired.
> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 84
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line84>
> >
> > I don't think the result of path::join is always expected to be absolute paths? I totally see where people want to join two relative paths together.
It isn't expected to be an absolute path. The code doesn't force that. It simply states that if the first portion of a path is an empty string (You pass the path {"", "foo", "bar"}, that path is absolute /foo/bar. If the first part isn't empty, then the path is relative {"foo","bar"} -> 'foo/bar'.
With vectors just appending the vectors works just as you expect when you then join() the resulting vector.
If you join an absolute path after another path, that will also come out as a developer expects (because join() is accepting of extra spaces in the middle of the path). The unit test has lots of examples that both relative and absolute paths are accepted.
They didn't particularly use to be before the rewrite in MESOS-1733. That used to assume they are always relative.
> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 121
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line121>
> >
> > Why does a absolute path give you two empty splits?
It allows us to tell "/" from just having an empty path "". Just an empty path is relative. Empty preceeding anything is a specification of an absolute path, starting at the root of a filesystem. Non-empty means that the path is relative. Admittedly this case could be reduced to {""}, which would have the same round trip effect. But the code is fairly thoroughly tested with the two parts, and that logically makes more sense to me. An empty directory name at the start means the root directory.
> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 123
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line123>
> >
> > So an empty string means slash then? if you want to keep the beginning slash(s) why not just use slash?
> > However, I think you have very specific use cases in mind implementing split and clean, and from the comments I can't really understand where you're coming from.
> > Can you comment on all these methods explaining what path::clean and path::split means and does?
Because joining if we use '/' is difficult. Esp. if you append an absolute path to the end of an absolute path {"a","b"} + {"/","a","b"}. Empty is what we should sanitize out anways ('a//b' people expect to equal 'a/b').
I can document a little more what clean does, although I don't think there is anything particularly useful to be said other than "It makes paths into a more normalized form, maintinaing meaning". (There is a reason it doesn't remove/sanitize out '..' or '.').
Split has name that matches content. I could write out every single base case it can explode to, but the behavior is writtent o roughly mirror join(). Inside the code of the function itself it comments why / what it is trying to clean at each step.
> On Oct. 16, 2014, 3:07 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp, line 25
> > <https://reviews.apache.org/r/26766/diff/1/?file=722432#file722432line25>
> >
> > Can we rename RoundTrip and RoundTripReduce to with names that resemable more with string functions? Like VerifySplitJoin? Also just two overloads.
They are specifically testing that the join and split round trip properly. That if you split a string you get to the expected intermediate. And if you join it you get back to where you started. They are much more about can we start with a string, parse it, and put it back together and get what we gave it (do a round trip), than verifying the operation of the individual components (although they also do that). Can you give any specific logic as to why 'VerifySplitJoin' is going to be easier for later developers to grok what is being tested by it?
I explicitly want them to be different names, because they have different testing behavior. RoundTrip() tests I get back exactly what I started with. If I don't it really is an error, not just "oh, I should add another parameter and the test case will pass". It has a very different meaning and test guarantee when the result ends up different than the source string, and I want it to be obvious that "For the string being tested, it is the case that we expect it to be simplified"
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56865
-----------------------------------------------------------
On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56865
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97302>
IMO calling asAbsolute("") sounds more like a bug then just returning "/".
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97306>
It's quite unclear what path::clean means, and have to read the tests to see what you're trying to do. When is this useful?
If we're trying to clean up user configuration, I rather we expose the error instead of trying to clean up mistakes.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97300>
I don't think the result of path::join is always expected to be absolute paths? I totally see where people want to join two relative paths together.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97309>
Why does a absolute path give you two empty splits?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97311>
So an empty string means slash then? if you want to keep the beginning slash(s) why not just use slash?
However, I think you have very specific use cases in mind implementing split and clean, and from the comments I can't really understand where you're coming from.
Can you comment on all these methods explaining what path::clean and path::split means and does?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97310>
Illegal based on?
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp
<https://reviews.apache.org/r/26766/#comment97301>
Can we rename RoundTrip and RoundTripReduce to with names that resemable more with string functions? Like VerifySplitJoin? Also just two overloads.
- Timothy Chen
On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56897
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97336>
Like you said { "", "" } really means the same as { "" }, so let's just simplify the code and use the while loop below.
- Timothy Chen
On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Cody Maloney <co...@mesosphere.io>.
> On Oct. 16, 2014, 5:01 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 123
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line123>
> >
> > So I'm coming from this is a shared function that is applicable for all paths in Mesos, and I won't really expect that "" == "/" in the path split, and assume I just get the directory names in the vector.
> >
> > Do you need to keep this info for your files.cpp?
Then all paths which are vectors __must__ either be absolute paths or the all __must__ be relative paths. There is no ability to represent both. I can't distinguish "/a/b" -> {"a","b"} from {"a/b"} -> {"a","b"}. I think there is considerable value in being able to represent both, and definitively know which is being represented. This parallels the variadic join in it's ability to represent both absolute paths and relative paths.
If I use a '/' to indicate the root rather than empty, then appending an absolute vector to non-absolute doesn't work. Empty string from what I can see makes the simplest/cleanest base case which needs the least special logic to allow both relative and absolute paths to be specified. It "just works" so long as people always use path::join() to join together their vector into a path string.
> On Oct. 16, 2014, 5:01 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 46
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line46>
> >
> > I see, but I'm still not clear why we're fixing up /a/b///c/d into /a/b/c/d for the user though?
Because real code in the wild will call the API with /a/b///c/d at some point. There is a lot of string joining code out there that does that.
Practically it is something we get for free / I'm not explicitly cleaning it up here. `strings::join(vector<>)` needs to do it to function properly when someone appends a absolute path to a relative path {"a"} + {"","b"} -> {"a","","b"} -> "a//b" isn't expected behavior from an end user (They really don't need to know about the empty chunk unless they are printing, in which case they should call path::join()). We don't lose anything by accepting this, it is the same behavior that people get from the open() syscall.
If someone passes in "/a/b//c/d" they are going to expect that the filesystem inside of /files/browse.json acts like it just passed that to opendir() and found the directory. That we have built a custom virtual filesystem is something they shouldn't be even remotely aware of.
We also have no good way of propogating that sort of an error out. If I don't clean / sanitize it, the developer gets a '404 FileNotFound'. When they go to the box, and 'cd' to that directory, it works. Just mesos looks/feels broken, and is hard to debug / figure out what is broken.
Making it so that it just works reduces user frustration, without costing us any additional code.
> On Oct. 16, 2014, 5:01 a.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 136
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line136>
> >
> > Why handle it specially here instead of just using the while below?
If you can make the while clean and handle all the cases, then definitely yes. This is checking that the first token is full, and the second token is empty, {"a", ""}, which doesn't logically make sense. We can't start the combining at index 1 always because of the {"", ""} case. Just informing the loop to start at 1 here seems easiest (The code could have the explicit erase removed as the loop will do it, but I think it makes the code more explicit what is happening).
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56885
-----------------------------------------------------------
On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review56885
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97323>
I see, but I'm still not clear why we're fixing up /a/b///c/d into /a/b/c/d for the user though?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97326>
So I'm coming from this is a shared function that is applicable for all paths in Mesos, and I won't really expect that "" == "/" in the path split, and assume I just get the directory names in the vector.
Do you need to keep this info for your files.cpp?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97327>
Why handle it specially here instead of just using the while below?
- Timothy Chen
On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Cody Maloney <co...@mesosphere.io>.
> On Oct. 17, 2014, 9:01 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 33
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line33>
> >
> > Hm.. what is asAbsolute doing? I would have expected this to be equivalent to `realpath()` from the signature name. That is, convert a relative path to an absolute path. However, this just prepends a slash? Seems like a fairly arbitrary path manipulation?
In all path manipulation code I've written going from a relative path to an absolute path is fairly common. I could make it so people could specify a prefix (So it effectively operates like --prefix in './configure') if that would make it make more sense as a helper function. Currently it is hard-coded that the prefix is '/'.
In terms of path specification, we always expect absolute paths which are rooted at '/'. In some of the tests we specify relative paths which don't begin with '/'. In some tests of a subset of the files API we use '/' prefixed paths. From the Mesos WebUI we always use absolute paths. This function makes it so I can handle all those cases without really thinking about it.
A name like 'ensureAbsolute' might be more relevant. For now I changed the name to 'prefix', because that is a filesystem path manipulation concept people are more familiar with.
> On Oct. 17, 2014, 9:01 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 48
> > <https://reviews.apache.org/r/26766/diff/1/?file=722431#file722431line48>
> >
> > This confuses me since it appears to be an identity function at first glance.
> >
> > For comparision, imagine if I was reading the following very similar identity function:
> >
> > ```
> > // Returns exactly what you pass in.
> > string identity(s)
> > {
> > return strings::join(",", strings::split(",", s));
> > }
> > ```
> >
> > Do we have the right abstractions here?
Added a comment to note what this does. path::join() performs a certain level of argument cleaning in it's implementation. The main operation that is happening here, is removing redundant slashes. Join must remove redundant slashes as part of it's regular operation to behave properly
The join() function behavior (Specifically the variadic one), is modeled after: https://docs.python.org/2/library/os.path.html#os.path.join
For path stuff join and split generally don't round trip, and making them do so presents a lot of unnecessary complexity to their callers. Rather than just the special empty path at the beginning to differentiate join / split, I have to keep all empty paths in the middle of strings (Problematic when files does it's browse), and that there is a trailing slash or not has to be indicated and dealt with as well.
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review57213
-----------------------------------------------------------
On Oct. 23, 2014, 4:52 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 4:52 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review57213
-----------------------------------------------------------
Hey Cody, just took a quick look at this, had a few high level questions below.
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97751>
Hm.. what is asAbsolute doing? I would have expected this to be equivalent to `realpath()` from the signature name. That is, convert a relative path to an absolute path. However, this just prepends a slash? Seems like a fairly arbitrary path manipulation?
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment97753>
This confuses me since it appears to be an identity function at first glance.
For comparision, imagine if I was reading the following very similar identity function:
```
// Returns exactly what you pass in.
string identity(s)
{
return strings::join(",", strings::split(",", s));
}
```
Do we have the right abstractions here?
- Ben Mahler
On Oct. 16, 2014, 8:53 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2014, 8:53 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/#review58113
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment99022>
Thanks for the updated comments and name changes, it's much easier to understand now!
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp
<https://reviews.apache.org/r/26766/#comment99019>
tokens.end() not needed?
It looks better from my view, I'll let Ben give a ship it before we put this in.
- Timothy Chen
On Oct. 23, 2014, 4:52 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26766/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 4:52 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
>
> The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
>
> Diff: https://reviews.apache.org/r/26766/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/
-----------------------------------------------------------
(Updated Oct. 23, 2014, 4:52 p.m.)
Review request for mesos, Ben Mahler and Timothy Chen.
Changes
-------
Rework / adjust function names to make more sense
Add some documentation comments about the behavior of each function
Simplify vector::split and vector::join so that the absolute path '/' is just {""}.
Bugs: MESOS-1878
https://issues.apache.org/jira/browse/MESOS-1878
Repository: mesos-git
Description
-------
Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
Diff: https://reviews.apache.org/r/26766/diff/
Testing
-------
make distcheck
Thanks,
Cody Maloney
Re: Review Request 26766: MESOS-1878: Add additional helper functions
to
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26766/
-----------------------------------------------------------
(Updated Oct. 16, 2014, 8:53 p.m.)
Review request for mesos, Ben Mahler and Timothy Chen.
Changes
-------
Add bmahler as a reviewer.
Going to add some docstring comments to the files and push up a new diff (As well as simplify the {"",""} to {""} in the next day or so.
Bugs: MESOS-1878
https://issues.apache.org/jira/browse/MESOS-1878
Repository: mesos-git
Description
-------
Adds 3 new functions: asAbsolute, clean, and split(). All three were hand-coded inside of mesos files (files/files.cpp). This puts them in a common place, and adds unit tests for their behavior.
The functions depend on eachother somewhat, so I pulled out the declarations to make them all forward declared.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 357a75a8bac497465671456aa9cd9181123cc635
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp aedf93573ea89e46bf7b7b91f2258049af2fd79f
Diff: https://reviews.apache.org/r/26766/diff/
Testing
-------
make distcheck
Thanks,
Cody Maloney