Re: Review Request 71882: Added a stout function to compute relative paths.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71878, 71879, 71880, 71881, 71882]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Dec. 9, 2019, 8:48 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71882/
> ---
> 
> (Updated Dec. 9, 2019, 8:48 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a stout function to compute relative paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71882/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71666: SSL Wrapper: Implemented send/recv and shutdown.

[Joseph Wu]

> On Dec. 5, 2019, 10:38 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 94 (patched)
> > 
> >
> > In this case, we don't really need an actor context, since there isn't 
> > any actor state associated with the compute thread. We really just want 
> > some context (any context) to dispatch the SSL-related functions onto, 
> > right?
> > 
> > It would make a bit more sense to me to dispatch these functions 
> > without specifying an actor, so that libprocess can run them wherever it 
> > pleases.
> > 
> > We could consider updating `loop()` to dispatch in all cases, even when 
> > no pid is specified. However, I do wonder if we're unknowingly depending on 
> > the existing behavior somewhere. In any case, changing loop to always 
> > `dispatch()` the iterate and body seems more desirable to me?
> > 
> > However, the `loop()` calls below aren't strictly necessary I think. We 
> > could accomplish the same thing with dispatches and chained continuations, 
> > so we could also just use `dispatch()` directly instead of `loop()`, that 
> > might be the simplest thing to do.
> > 
> > WDYT?
> 
> Joseph Wu wrote:
> I think a UPID/actor is required for any dispatching/looping on 
> libprocess worker threads, so this variable would still exist regardless of 
> how the loops are implemented.
> 
> The alternative is to run everything on the event loop thread (or special 
> threads we spin up/acquire out of band?).
> 
> Greg Mann wrote:
> Ah I was remembering the version of `defer()` which can be invoked 
> without a pid: 
> https://github.com/apache/mesos/blob/925ad30c0f3b249afe74bdeb1921c5fdbf1c5886/3rdparty/libprocess/include/process/defer.hpp#L275-L283
> 
> Actually, I wish we had an overload of `dispatch()` that did something 
> similar. In any case, the `defer()` overload might work here, WDYT?
> 
> Joseph Wu wrote:
> That overload of `defer()` ends up running things on a thread_local UPID:
> ```
> // Per thread executor pointer. We use a pointer to lazily construct the
> // actual executor.
> extern thread_local Executor* _executor_;
> 
> #define __executor__\
>   (_executor_ == nullptr ? _executor_ = new Executor() : _executor_)
> ```
> 
> In this case, I think we would end up constructing a single 
> `__executor__` object on the EventLoop thread (since that is where the 
> `defer()` is called), so all socket IO would end up deferred onto the same 
> UPID.
> 
> Greg Mann wrote:
> Yea, I think you're right. That seems a bit better to me than 
> constructing a one-off UPID specifically for SSL work, WDYT?

I mean the _same_ UPID (only one).  Just one actor would handle all the SSL 
work.


- Joseph


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


On Nov. 19, 2019, 4:29 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> ---
> 
> (Updated Nov. 19, 2019, 4:29 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This completes a fully functional client-side SSL socket.
> 
> Needs a bit of cleanup and more error handling though.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71666/diff/6/
> 
> 
> Testing
> ---
> 
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
> 
> Running libprocess-tests yields:
> ```
> [  FAILED  ] SSLTest.ValidDowngrade
> [  FAILED  ] SSLTest.ValidDowngradeEachProtocol
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71666: SSL Wrapper: Implemented send/recv and shutdown.

[Greg Mann]

> On Dec. 5, 2019, 6:38 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 94 (patched)
> > 
> >
> > In this case, we don't really need an actor context, since there isn't 
> > any actor state associated with the compute thread. We really just want 
> > some context (any context) to dispatch the SSL-related functions onto, 
> > right?
> > 
> > It would make a bit more sense to me to dispatch these functions 
> > without specifying an actor, so that libprocess can run them wherever it 
> > pleases.
> > 
> > We could consider updating `loop()` to dispatch in all cases, even when 
> > no pid is specified. However, I do wonder if we're unknowingly depending on 
> > the existing behavior somewhere. In any case, changing loop to always 
> > `dispatch()` the iterate and body seems more desirable to me?
> > 
> > However, the `loop()` calls below aren't strictly necessary I think. We 
> > could accomplish the same thing with dispatches and chained continuations, 
> > so we could also just use `dispatch()` directly instead of `loop()`, that 
> > might be the simplest thing to do.
> > 
> > WDYT?
> 
> Joseph Wu wrote:
> I think a UPID/actor is required for any dispatching/looping on 
> libprocess worker threads, so this variable would still exist regardless of 
> how the loops are implemented.
> 
> The alternative is to run everything on the event loop thread (or special 
> threads we spin up/acquire out of band?).
> 
> Greg Mann wrote:
> Ah I was remembering the version of `defer()` which can be invoked 
> without a pid: 
> https://github.com/apache/mesos/blob/925ad30c0f3b249afe74bdeb1921c5fdbf1c5886/3rdparty/libprocess/include/process/defer.hpp#L275-L283
> 
> Actually, I wish we had an overload of `dispatch()` that did something 
> similar. In any case, the `defer()` overload might work here, WDYT?
> 
> Joseph Wu wrote:
> That overload of `defer()` ends up running things on a thread_local UPID:
> ```
> // Per thread executor pointer. We use a pointer to lazily construct the
> // actual executor.
> extern thread_local Executor* _executor_;
> 
> #define __executor__\
>   (_executor_ == nullptr ? _executor_ = new Executor() : _executor_)
> ```
> 
> In this case, I think we would end up constructing a single 
> `__executor__` object on the EventLoop thread (since that is where the 
> `defer()` is called), so all socket IO would end up deferred onto the same 
> UPID.

Yea, I think you're right. That seems a bit better to me than constructing a 
one-off UPID specifically for SSL work, WDYT?


- Greg


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


On Nov. 20, 2019, 12:29 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71666/
> ---
> 
> (Updated Nov. 20, 2019, 12:29 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10010
> https://issues.apache.org/jira/browse/MESOS-10010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This completes a fully functional client-side SSL socket.
> 
> Needs a bit of cleanup and more error handling though.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71666/diff/6/
> 
> 
> Testing
> ---
> 
> ```
> cmake --build . --target libprocess-tests
> libprocess-tests
> ```
> 
> Running libprocess-tests yields:
> ```
> [  FAILED  ] SSLTest.ValidDowngrade
> [  FAILED  ] SSLTest.ValidDowngradeEachProtocol
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71882: Added a stout function to compute relative paths.

[Benjamin Bannier]

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

(Updated Dec. 9, 2019, 5:48 p.m.)


Review request for mesos and Benno Evers.


Bugs: MESOS-10062
https://issues.apache.org/jira/browse/MESOS-10062


Repository: mesos


Description
---

Added a stout function to compute relative paths.


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
  3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 


Diff: https://reviews.apache.org/r/71882/diff/2/

Changes: https://reviews.apache.org/r/71882/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71882: Added a stout function to compute relative paths.

[Benjamin Bannier]

> On Dec. 9, 2019, 1:09 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 21 (patched)
> > 
> >
> > This doesn't seem to be necessary anymore?

This is required for e.g., `CHECK_EQ` (now).


> On Dec. 9, 2019, 1:09 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 551 (patched)
> > 
> >
> > I found the description below easier to parse:
> > 
> > > Relative paths can only be computed between paths which are either 
> > both absolute or both relative.

Fixed.


> On Dec. 9, 2019, 1:09 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 590 (patched)
> > 
> >
> > How about
> > 
> > the range of `base`
> > 
> > the `base` range
> > 
> > the range `[base.begin(), base.end())`
> > 
> > to avoid the awkward backtick-single-quote?

Went with your first suggestion.


- Benjamin


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


On Dec. 9, 2019, 5:48 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71882/
> ---
> 
> (Updated Dec. 9, 2019, 5:48 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a stout function to compute relative paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71882/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71878: Added iteration support to stout's Path.

[Benjamin Bannier]

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

(Updated Dec. 9, 2019, 5:48 p.m.)


Review request for mesos and Benno Evers.


Changes
---

Correct changeset


Bugs: MESOS-10062
https://issues.apache.org/jira/browse/MESOS-10062


Repository: mesos


Description
---

Added iteration support to stout's Path.


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
  3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 


Diff: https://reviews.apache.org/r/71878/diff/3/

Changes: https://reviews.apache.org/r/71878/diff/2-3/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71878: Added iteration support to stout's Path.

[Benjamin Bannier]

> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 388 (patched)
> > 
> >
> > This feels a bit more "common" to me:
> > 
> > struct PathComponentIterator {
> > // ..
> > };
> > 
> > typedef const PathComponentIterator const_iterator;
> > 
> > What do you think?

I'd vote to not do this as we do not need it.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 389 (patched)
> > 
> >
> > Using `std::iterator` is going to be deprecated in C++17, apparently 
> > the preferred alternative is to just create typedefs for `value_type`, etc. 
> > in the iterator class directly.

I'll do that since it also documents more what is happening in a more obvious 
way.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 437 (patched)
> > 
> >
> > I think this needs to be a loop, consider e.g. `/home/foobar`

That would be path normalization which I didn't want to do in this class as it 
has a number of other complications. For a path like `/home/foo//bar` we would 
like to generate elements `{"home", "foo", "", "bar"}`. I'll clean up remnants 
of my attempts in this class here and in tests.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 454 (patched)
> > 
> >
> > Should we also check `path == other.path`? Or maybe even `CHECK(path == 
> > other.path)`?

I'll end the hard `CHECK` once I have gotten rid of the end sentinel.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 469 (patched)
> > 
> >
> > I don't completely understand why we need a sentinel, isn't 
> > `path->end()` suitable for everything we need to do?

I removed as per our offline discussion.


- Benjamin


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


On Dec. 9, 2019, 5:38 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71878/
> ---
> 
> (Updated Dec. 9, 2019, 5:38 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added iteration support to stout's Path.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71878/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71878: Added iteration support to stout's Path.

[Benjamin Bannier]

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

(Updated Dec. 9, 2019, 5:38 p.m.)


Review request for mesos and Benno Evers.


Bugs: MESOS-10062
https://issues.apache.org/jira/browse/MESOS-10062


Repository: mesos


Description
---

Added iteration support to stout's Path.


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
  3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 


Diff: https://reviews.apache.org/r/71878/diff/2/

Changes: https://reviews.apache.org/r/71878/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71833: Created unix domain socket on agent startup.

[Benno Evers]

> On Dec. 5, 2019, 3:12 p.m., Benjamin Bannier wrote:
> > src/tests/cluster.cpp
> > Lines 630 (patched)
> > 
> >
> > There is another, currently unhandled branch where 
> > `domain_socket_location` exists, but is not a socket. Either we handle that 
> > case as well, or just check if _something_ exists where we'd expect the 
> > socket and  let this fail later.

I think the latter variant is what the code does, no? If there exists another 
file that is not a socket, we should end up with an error upon calling `bind()`.


- Benno


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


On Dec. 3, 2019, 6:30 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71833/
> ---
> 
> (Updated Dec. 3, 2019, 6:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-10036
> https://issues.apache.org/jira/browse/MESOS-10036
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent code to check whether domain socket support is
> enabled, and if so to create or open the socket at
> MESOS_DOMAIN_SOCKET_LOCATION.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448 
>   src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7 
>   src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb 
>   src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 
>   src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7 
>   src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0 
> 
> 
> Diff: https://reviews.apache.org/r/71833/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71866: Added new chmod() function to stout.

[Benno Evers]

> On Dec. 5, 2019, 12:43 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/os/posix/chmod.hpp
> > Lines 34 (patched)
> > 
> >
> > Let's return a `ErrnoError(errno)` here.
> > 
> > Do we need to include `cstdio`/`stdio.h` for `::strerror`?

We don't, `strerror()` should be defined in `string.h`/``.


- Benno


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


On Dec. 3, 2019, 6:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71866/
> ---
> 
> (Updated Dec. 3, 2019, 6:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new functions `os::chmod()` and `os::posix::chmod()` that
> are wrapping the POSIX `::chmod()` function.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/chmod.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/chmod.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/chmod.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71866/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71878: Added iteration support to stout's Path.

[Benno Evers]

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




3rdparty/stout/include/stout/path.hpp
Lines 388 (patched)


This feels a bit more "common" to me:

struct PathComponentIterator {
// ..
};

typedef const PathComponentIterator const_iterator;

What do you think?



3rdparty/stout/include/stout/path.hpp
Lines 389 (patched)


Using `std::iterator` is going to be deprecated in C++17, apparently the 
preferred alternative is to just create typedefs for `value_type`, etc. in the 
iterator class directly.



3rdparty/stout/include/stout/path.hpp
Lines 391 (patched)


Shouldn't `value_type` be `std::string`? (or the `string_view`-like class 
from the TODO below)



3rdparty/stout/include/stout/path.hpp
Lines 437 (patched)


I think this needs to be a loop, consider e.g. `/home/foobar`



3rdparty/stout/include/stout/path.hpp
Lines 454 (patched)


Should we also check `path == other.path`? Or maybe even `CHECK(path == 
other.path)`?



3rdparty/stout/include/stout/path.hpp
Lines 469 (patched)


I don't completely understand why we need a sentinel, isn't `path->end()` 
suitable for everything we need to do?



3rdparty/stout/include/stout/path.hpp
Lines 479 (patched)


Why not store a `Path*` here? Then we would not need to have an extra copy 
of `separator` in the iterator.


- Benno Evers


On Dec. 5, 2019, 12:48 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71878/
> ---
> 
> (Updated Dec. 5, 2019, 12:48 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added iteration support to stout's Path.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71878/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71882: Added a stout function to compute relative paths.

[Benno Evers]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/path.hpp
Lines 21 (patched)


This doesn't seem to be necessary anymore?



3rdparty/stout/include/stout/path.hpp
Lines 551 (patched)


I found the description below easier to parse:

> Relative paths can only be computed between paths which are either both 
absolute or both relative.



3rdparty/stout/include/stout/path.hpp
Lines 556 (patched)


I personally don't really mind, but our styleguide doesn't seem to like 
this:

> Some trailing underscores are used to distinguish between similar 
variables in the same scope (think prime symbols), but this should be avoided 
as much as possible, including removing existing instances in the code base.



3rdparty/stout/include/stout/path.hpp
Lines 590 (patched)


How about

the range of `base`

the `base` range

the range `[base.begin(), base.end())`

to avoid the awkward backtick-single-quote?


- Benno Evers


On Dec. 5, 2019, 12:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71882/
> ---
> 
> (Updated Dec. 5, 2019, 12:49 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a stout function to compute relative paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71882/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71881: Allowed specifying path separator in a `path::join` overload.

[Benno Evers]

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


Ship it!




Ship It!

- Benno Evers


On Dec. 5, 2019, 12:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71881/
> ---
> 
> (Updated Dec. 5, 2019, 12:49 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed specifying path separator in a `path::join` overload.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
> 
> 
> Diff: https://reviews.apache.org/r/71881/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71880: Renamed stout's path-related absolute functions to is_absolute.

[Benno Evers]

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


Ship it!




Ship It!

- Benno Evers


On Dec. 5, 2019, 12:48 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71880/
> ---
> 
> (Updated Dec. 5, 2019, 12:48 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed stout's path-related absolute functions to is_absolute.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp d13869db7c9ead178cb52eec5d0700931f987161 
>   src/hdfs/hdfs.cpp 3947b69c965cfb7289f563202288ea6e4979d942 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 027ceaac7cc3db100186aebf2c3ebfd1e06137d9 
>   src/slave/container_loggers/logrotate.hpp 
> c6dbd61a5083dbb5c52bfb514ea7979a7a51158b 
>   src/slave/containerizer/fetcher.cpp 
> 3a5a74bf4e64eeaed161bb2e7875e185f2271aef 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> e4a19c471b3b5442e5fdc15b31b1799a6638a2d0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ab192305b53c829fbf6cbf6367ce1686507f9c3d 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> 4d8047b1f5966428911d095c734a0155b1f5148a 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> a20f7ac527a0ae68a03ec7095c09def5b34f5109 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> b1112529ac1abe01368fda501a675e7b3e6388ef 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 45bd143bbc829c105dc1f1c8c88f0168aeaf53a0 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 91d701376069a65046e487a5070325e3bc61deb8 
>   src/slave/containerizer/mesos/launch.cpp 
> 1f8dc1ae2e3d750d56973f9efc1ccbd4b1e50eba 
>   src/slave/paths.cpp 5afcc7e104aa4232554d54e2f290aede4ebba3c1 
>   src/tests/storage_local_resource_provider_tests.cpp 
> d4a73c34afb034ac35a3e305e603c61a03b99b5d 
>   src/uri/fetchers/docker.cpp 1aa3def8362269e94c7f0bb05f9aa10049bf4af4 
> 
> 
> Diff: https://reviews.apache.org/r/71880/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71879: Renamed stout's path-related absolute functions to is_absolute.

[Benno Evers]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/path.hpp
Line 154 (original), 154 (patched)


Ideally we'd still keep a deprecated `absolute()` function around, to not 
break backwards compatibility.

But I'm not sure what our current policy on stout backwards compatibility 
is, if we're regularly doing similar refactorings in stout, feel free to drop 
this issue.


- Benno Evers


On Dec. 5, 2019, 12:48 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71879/
> ---
> 
> (Updated Dec. 5, 2019, 12:48 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed stout's path-related absolute functions to is_absolute.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/longpath.hpp 
> 499eef30a3d16ac1f6c2e3334ef773e91e987a45 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp 
> 631babc17fff978b4e2048e5041a9f8db9b282b2 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp 
> 34723bce9a151582de481d63a865509d4d29c02c 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71879/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>