Re: Review Request 55438: Fixed flakiness in SlaveRecoveryTest.RegisterDisconnectedSlave.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [55438, 55437]

Failed command: python support/apply-reviews.py -n -r 55437

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 349, in 
reviewboard()
  File "support/apply-reviews.py", line 328, in reviewboard
apply_review()
  File "support/apply-reviews.py", line 121, in apply_review
fetch_patch()
  File "support/apply-reviews.py", line 150, in fetch_patch
r = urllib2.urlopen(patch_url(), context=ssl_create_default_context())
  File "support/apply-reviews.py", line 131, in ssl_create_default_context
context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
AttributeError: 'module' object has no attribute 'SSLContext'
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
  File "support/apply-reviews.py", line 119, in 
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '55437.patch'
Error in sys.exitfunc:
Traceback (most recent call last):
  File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
func(*targs, **kargs)
  File "support/apply-reviews.py", line 119, in 
atexit.register(lambda: os.remove('%s.patch' % patch_id()))
OSError: [Errno 2] No such file or directory: '55437.patch'

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16689/console

- Mesos ReviewBot


On Jan. 12, 2017, 12:38 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55438/
> ---
> 
> (Updated Jan. 12, 2017, 12:38 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6911
> https://issues.apache.org/jira/browse/MESOS-6911
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed flakiness in SlaveRecoveryTest.RegisterDisconnectedSlave.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 9323cbb74dd555fe6c44af1ef6da70e0c2d76dcb 
> 
> Diff: https://reviews.apache.org/r/55438/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Ran ~5000 test iterations on a slow VM that previously exhibited flakiness 
> for this test.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 55435: Windows: Fixed the locale guard in jsonify.hpp.

[Michael Park]

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




3rdparty/stout/include/stout/jsonify.hpp (lines 75 - 78)


Shouldn't we call `_configthreadlocale(_DISABLE_PER_THREAD_LOCALE);` here 
to recover the original `setlocale` behavior?


- Michael Park


On Jan. 11, 2017, 4:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55435/
> ---
> 
> (Updated Jan. 11, 2017, 4:56 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and 
> Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changing locales in Windows is mainly accomplished via `setlocale`,
> which takes a bit mask (with constants named slightly differently
> than in the POSIX headers) and a `char*`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 3c48046e087de2a66139a31449327fd94c149371 
> 
> Diff: https://reviews.apache.org/r/55435/diff/
> 
> 
> Testing
> ---
> 
> Windows:
> 
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
> 3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55238, 55239, 55240, 55241, 55242]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Jan. 12, 2017, 12:29 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55242/
> ---
> 
> (Updated Jan. 12, 2017, 12:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reimplement os::chown() to use fts(3) rather than sometimes spawning
> chown(1). This removes the use of the shell and the corresponding
> need to sanitize path arguments.  It also enables us to provide
> consistent handling of symbolic links for the recursive and
> non-recursive cases. We ensure that symlinks are never followed
> and that we always change the ownership of the link itself, not
> its referent.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/chown.hpp 
> c82e2e574019c5ee5f17ea105a6d225006388a45 
>   3rdparty/stout/include/stout/os/posix/stat.hpp 
> 1ab20e75fc18b336162b62e2f4f23b68f6685183 
>   3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 
> 
> Diff: https://reviews.apache.org/r/55242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55040: CMake: Added correct values for some build flags.

[Joseph Wu]

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


Ship it!




I can tweak the below.


cmake/CompilationConfigure.cmake (lines 250 - 262)


I'd group the PicoJSON definitions together and move the comment up 
appropriately.


- Joseph Wu


On Dec. 26, 2016, 1:54 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55040/
> ---
> 
> (Updated Dec. 26, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-5455
> https://issues.apache.org/jira/browse/MESOS-5455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, per MESOS-5455, CMake does not implement certain build flags
> as the autotools solution does. For example, `BUILD_DATE` is hard coded,
> and the `BUILD_USER` flags is always "frank". This commit will add
> correct values for all such flags, with the exception of one,
> `BUILD_FLAGS`.
> 
> In addition, this commit will take this opportunity to clean up the
> organization of the build flags. Currently, we call `add_definitions`
> individually for each flag we wish to define. This commit will introduce
> a new variable, `MESOS_CPPFLAGS`, which holds the build flags for the
> project. We call `add_definitions` once, when `MESOS_CPPFLAGS` is
> completely populated.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 1349afb44df0d922bf7f87ed1c73076590957089 
>   src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
> 
> Diff: https://reviews.apache.org/r/55040/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55039: CMake: Removed unnecessary calls to FindApr and FindSvn.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Dec. 26, 2016, 1:54 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55039/
> ---
> 
> (Updated Dec. 26, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake: Removed unnecessary calls to FindApr and FindSvn.
> 
> `StoutConfigure` is intended to handle these calls.
> 
> 
> Diffs
> -
> 
>   src/master/cmake/MasterConfigure.cmake 
> a15f6890f088c92c8f275bc1ceec17cf6ad0952c 
>   src/slave/cmake/AgentConfigure.cmake 
> fcac980f55cf6f7a582053b786cf73c6515e3a0b 
> 
> Diff: https://reviews.apache.org/r/55039/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55038: CMake: Stop searching for SVN and APR after we've found them.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Dec. 26, 2016, 1:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55038/
> ---
> 
> (Updated Dec. 26, 2016, 1:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, the CMake build system employs scripts to find system
> installations of libsvn and libapr on Unix machines. These scripts will
> always repeat the search from scratch, independent of whether we've
> already searched for these libraries.
> 
> This commit will cause these scripts to terminate early if we've already
> searched and found these libraries.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/FindApr.cmake 98adeabc19ac18a6519d72a43e7b7adeb68579de 
>   3rdparty/stout/cmake/FindSvn.cmake e85e882db3b829e61e82945b056eb3cc7f2a803e 
> 
> Diff: https://reviews.apache.org/r/55038/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Joseph Wu]

> On Dec. 27, 2016, 6:12 a.m., Till Toenshoff wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 148
> > 
> >
> > By not adding `syswow64` we are excluding 32bit runnables, is this 
> > intentional and documented? Is this still a thing on windows?

Actually, in the current implementation of subprocess on Windows, any 
user-specified environment variables that conflict with system environment 
variables will be overridden at subprocess launch.  This means you can specify 
a `PATH=nowhere` and we'll just ignore it.

At some point, we may loosen this behavior, as we only did this because the 
test coverage of the Windows code at the time was nil, and a bunch of other 
changes kept breaking the environment variables on Windows :)


- Joseph


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


On Dec. 26, 2016, 1:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Dec. 26, 2016, 1:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `default_path` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55343: Fixed SSL socket 'shutdown()'.

[Greg Mann]

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

(Updated Jan. 12, 2017, 1:02 a.m.)


Review request for mesos, Benjamin Hindman and Joseph Wu.


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


Repository: mesos


Description
---

Recently, a change was made to the signature of
`Socket::shutdown`, but the corresponding override in
`LibeventSSLSocketImpl` was not updated, so that the
implementation-specific method is no longer being
executed. Further, the SSL socket's `shutdown` code
did not actually shutdown the socket; rather, the
shutdown was performed in the destructor.

This patch updates the function's signature to match
that of the base class's method, adds the `override`
specifier to the implemention's method declaration,
and updates the function to properly shutdown the
SSL socket.


Diffs
-

  3rdparty/libprocess/include/process/socket.hpp 
87966155aa21328db51796b2ae0a883054c00457 
  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

Diff: https://reviews.apache.org/r/55343/diff/


Testing (updated)
---

`3rdparty/libprocess/libprocess-tests 
--gtest_filter="Encryption/NetSocketTest.EOFBeforeRecv/0:Encryption/NetSocketTest.EOFAfterRecv/0"
 --gtest_repeat=-1 --gtest_break_on_failure`

NOTE: currently the above command exposes a file descriptor leak related to the 
SSL socket.


Thanks,

Greg Mann

Re: Review Request 53803: Added new libprocess socket tests.

[Greg Mann]

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

(Updated Jan. 12, 2017, 12:57 a.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description
---

This patch adds NetSocketTest.EOFBeforeRecv and
NetSocketTest.EOFAfterRecv to verify that EOFs are
reliably received whether or not there is a pending recv()
request at the time the EOF is received.


Diffs
-

  3rdparty/libprocess/src/tests/socket_tests.cpp 
44c3c9adc39702dd598aa0105088517df601bbda 

Diff: https://reviews.apache.org/r/53803/diff/


Testing
---

Testing details are at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 55343: Fixed SSL socket 'shutdown()'.

[Greg Mann]

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

(Updated Jan. 12, 2017, 12:57 a.m.)


Review request for mesos, Benjamin Hindman and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Recently, a change was made to the signature of
`Socket::shutdown`, but the corresponding override in
`LibeventSSLSocketImpl` was not updated, so that the
implementation-specific method is no longer being
executed. Further, the SSL socket's `shutdown` code
did not actually shutdown the socket; rather, the
shutdown was performed in the destructor.

This patch updates the function's signature to match
that of the base class's method, adds the `override`
specifier to the implemention's method declaration,
and updates the function to properly shutdown the
SSL socket.


Diffs
-

  3rdparty/libprocess/include/process/socket.hpp 
87966155aa21328db51796b2ae0a883054c00457 
  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

Diff: https://reviews.apache.org/r/55343/diff/


Testing (updated)
---

`bin/mesos-tests.sh 
--gtest_filter="Encryption/NetSocketTest.EOFBeforeRecv/0:Encryption/NetSocketTest.EOFAfterRecv/0"
 --gtest_repeat=-1 --gtest_break_on_failure`

NOTE: currently the above command exposes a file descriptor leak related to the 
SSL socket.


Thanks,

Greg Mann

Re: Review Request 55435: Windows: Fixed the locale guard in jsonify.hpp.

[Joseph Wu]

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

(Updated Jan. 11, 2017, 4:56 p.m.)


Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and 
Michael Park.


Changes
---

Reduced the number of `ifdef`s (3 -> 1)and made the locale guard thread safe.


Repository: mesos


Description
---

Changing locales in Windows is mainly accomplished via `setlocale`,
which takes a bit mask (with constants named slightly differently
than in the POSIX headers) and a `char*`.


Diffs (updated)
-

  3rdparty/stout/include/stout/jsonify.hpp 
3c48046e087de2a66139a31449327fd94c149371 

Diff: https://reviews.apache.org/r/55435/diff/


Testing
---

Windows:

msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"


Thanks,

Joseph Wu

Review Request 55437: Cleaned up SlaveRecoveryTest.RegisterDisconnectedSlave.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Cleaned up SlaveRecoveryTest.RegisterDisconnectedSlave.


Diffs
-

  src/tests/slave_recovery_tests.cpp 9323cbb74dd555fe6c44af1ef6da70e0c2d76dcb 

Diff: https://reviews.apache.org/r/55437/diff/


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 55438: Fixed flakiness in SlaveRecoveryTest.RegisterDisconnectedSlave.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Fixed flakiness in SlaveRecoveryTest.RegisterDisconnectedSlave.


Diffs
-

  src/tests/slave_recovery_tests.cpp 9323cbb74dd555fe6c44af1ef6da70e0c2d76dcb 

Diff: https://reviews.apache.org/r/55438/diff/


Testing
---

`make check`

Ran ~5000 test iterations on a slow VM that previously exhibited flakiness for 
this test.


Thanks,

Neil Conway

Re: Review Request 55241: Stop using os::system to validate perf event names.

[James Peach]

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

(Updated Jan. 12, 2017, 12:28 a.m.)


Review request for mesos, Benjamin Mahler, Greg Mann, and Jiang Yan Xu.


Changes
---

Rebase and address review feedback.


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


Repository: mesos


Description
---

Stop using os::system to validate perf event names.


Diffs (updated)
-

  src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 

Diff: https://reviews.apache.org/r/55241/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[James Peach]

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

(Updated Jan. 12, 2017, 12:29 a.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Rebase and address review feedback.


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


Repository: mesos


Description (updated)
---

Reimplement os::chown() to use fts(3) rather than sometimes spawning
chown(1). This removes the use of the shell and the corresponding
need to sanitize path arguments.  It also enables us to provide
consistent handling of symbolic links for the recursive and
non-recursive cases. We ensure that symlinks are never followed
and that we always change the ownership of the link itself, not
its referent.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/posix/chown.hpp 
c82e2e574019c5ee5f17ea105a6d225006388a45 
  3rdparty/stout/include/stout/os/posix/stat.hpp 
1ab20e75fc18b336162b62e2f4f23b68f6685183 
  3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 

Diff: https://reviews.apache.org/r/55242/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55240: Stop using os::system to copy local files.

[James Peach]

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

(Updated Jan. 12, 2017, 12:27 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebase and address review feedback.


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


Repository: mesos


Description
---

Stop using os::system to copy local files.


Diffs (updated)
-

  src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 

Diff: https://reviews.apache.org/r/55240/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55239: Stop using os::system to extract fetcher archives.

[James Peach]

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

(Updated Jan. 12, 2017, 12:27 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebase and address review feedback.


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


Repository: mesos


Description
---

Stop using os::system to extract fetcher archives.


Diffs (updated)
-

  src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 

Diff: https://reviews.apache.org/r/55239/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55238: Use os::spawn in the CNI isolator.

[James Peach]

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

(Updated Jan. 12, 2017, 12:26 a.m.)


Review request for mesos, Avinash sridharan and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Use os::spawn in the CNI isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 

Diff: https://reviews.apache.org/r/55238/diff/


Testing
---

`sudo make check` (Fedora 25)


Thanks,

James Peach

Re: Review Request 55343: Fixed SSL socket 'shutdown()'.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: [55343, 53803]

Error:
Circular dependency detected for review 53803.Please fix the 'depends_on' field.

- Mesos ReviewBot


On Jan. 11, 2017, 11:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55343/
> ---
> 
> (Updated Jan. 11, 2017, 11:10 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed SSL socket 'shutdown()'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> 87966155aa21328db51796b2ae0a883054c00457 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
> 
> Diff: https://reviews.apache.org/r/55343/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 55435: Windows: Fixed the locale guard in jsonify.hpp.

[Joseph Wu]

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

Review request for mesos, Alexander Rojas, Daniel Pravat, Alex Clemmer, and 
Michael Park.


Repository: mesos


Description
---

Changing locales in Windows is mainly accomplished via `setlocale`,
which takes a bit mask (with constants named slightly differently
than in the POSIX headers) and a `char*`.


Diffs
-

  3rdparty/stout/include/stout/jsonify.hpp 
3c48046e087de2a66139a31449327fd94c149371 

Diff: https://reviews.apache.org/r/55435/diff/


Testing
---

Windows:

msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout-tests
3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*Json*"


Thanks,

Joseph Wu

Re: Review Request 55343: Fixed SSL socket 'shutdown()'.

[Greg Mann]

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

(Updated Jan. 11, 2017, 11:10 p.m.)


Review request for mesos and Greg Mann.


Repository: mesos


Description
---

Fixed SSL socket 'shutdown()'.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 
87966155aa21328db51796b2ae0a883054c00457 
  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
57eaf4f607d0628db466cc1a139772eeeaa51136 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
0e292a8b0d470f4e199db08f09a0c863d73c 

Diff: https://reviews.apache.org/r/55343/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 55030: CMake: Added source groups for libprocess build.

[Alex Clemmer]

> On Jan. 11, 2017, 10:23 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/tests/CMakeLists.txt, lines 80-81
> > 
> >
> > What's the effect of adding these `source_groups` in multiple CMake 
> > lists?
> > 
> > Is the only effect that each subproject in MSVC will show the files in 
> > an organized way?

The effect is that they're organized in every IDE, not just VS.

(Also, please let me know if the commit message needs to be massaged for this 
to make sense.)


- Alex


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


On Dec. 25, 2016, 5:39 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55030/
> ---
> 
> (Updated Dec. 25, 2016, 5:39 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3542
> https://issues.apache.org/jira/browse/MESOS-3542
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in IDEs such as XCode and Visual Studio, all mesos source
> files are grouped into one huge folder, with no attempt made to reflect
> the hierarchy of folders that exist on disk.
> 
> This commit groups libprocess files into relevant source groups so that
> they appear in a sensible directory structure in these IDEs.
> 
> In addition to beginning to directly address MESOS-3542, this is also
> (indirectly) the first step towards addressing the problem of breaking
> libmesos up into smaller binaries (MESOS-3542).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 873f41d844faa0d442f77411e94314a89be5f046 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 60f0e76dfd237d9a12a366b413802d1a96892b55 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> 4b80c397381ca2c869cd6eb7507bb9df94ce3623 
> 
> Diff: https://reviews.apache.org/r/55030/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55029: CMake: Added source groups to Stout build.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Dec. 24, 2016, 9:38 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55029/
> ---
> 
> (Updated Dec. 24, 2016, 9:38 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3542
> https://issues.apache.org/jira/browse/MESOS-3542
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in IDEs such as XCode and Visual Studio, all mesos source
> files are grouped into one huge folder, with no attempt made to reflect
> the hierarchy of folders that exist on disk.
> 
> This commit groups Stout files into relevant source groups so that they
> appear in a sensible directory structure in these IDEs.
> 
> In addition to beginning to directly address MESOS-3542, this is also
> (indirectly) the first step towards addressing the problem of breaking
> libmesos up into smaller binaries (MESOS-3542).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> 04e0f2f0d464d6740055eb1db2a8d41349879171 
>   3rdparty/stout/tests/CMakeLists.txt 
> a09693d4b9a70faf1e4e337ec71ead2f0bbab0a2 
> 
> Diff: https://reviews.apache.org/r/55029/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55030: CMake: Added source groups for libprocess build.

[Joseph Wu]

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




3rdparty/libprocess/cmake/ProcessConfigure.cmake (lines 59 - 72)


s/Process/Libprocess/
^ Not in the path, only in the first argument of `source_group`.

Even if the "lib" part is a unix semantic, the full name of this 3rdparty 
library is "libprocess".



3rdparty/libprocess/src/CMakeLists.txt (lines 72 - 78)


There are two other SSL .cpp files:

* openssl.cpp
* libevent_ssl_socket.cpp

Note that these are not in the `ssl` subfolder.



3rdparty/libprocess/src/CMakeLists.txt (line 140)


s/Process/Libprocess/



3rdparty/libprocess/src/tests/CMakeLists.txt (lines 80 - 81)


What's the effect of adding these `source_groups` in multiple CMake lists?

Is the only effect that each subproject in MSVC will show the files in an 
organized way?


- Joseph Wu


On Dec. 24, 2016, 9:39 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55030/
> ---
> 
> (Updated Dec. 24, 2016, 9:39 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3542
> https://issues.apache.org/jira/browse/MESOS-3542
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in IDEs such as XCode and Visual Studio, all mesos source
> files are grouped into one huge folder, with no attempt made to reflect
> the hierarchy of folders that exist on disk.
> 
> This commit groups libprocess files into relevant source groups so that
> they appear in a sensible directory structure in these IDEs.
> 
> In addition to beginning to directly address MESOS-3542, this is also
> (indirectly) the first step towards addressing the problem of breaking
> libmesos up into smaller binaries (MESOS-3542).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 873f41d844faa0d442f77411e94314a89be5f046 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 60f0e76dfd237d9a12a366b413802d1a96892b55 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> 4b80c397381ca2c869cd6eb7507bb9df94ce3623 
> 
> Diff: https://reviews.apache.org/r/55030/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55026: Windows: Shortened test directories on Windows to avoid path errors.

[Alex Clemmer]

> On Jan. 11, 2017, 10:03 p.m., Joseph Wu wrote:
> > LGTM.
> > 
> > As some background (and for your information), these temporary directories 
> > are created in tests like `environment->mktemp()`.  This is something we 
> > use for debugging, as having the test name in the temporary directory makes 
> > tracking down failed/crashed tests easier.  With this change, it will be 
> > even more important for tests to be run with `--verbose`; as long as tests 
> > are verbose, we can track down the appropriate temporary folders for almost 
> > all debugging purposes.

100% agreed. We need to solve the path issue and go back to the way it was. 
Thanks for doing due diligence. :)


- Alex


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


On Dec. 24, 2016, 11:14 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55026/
> ---
> 
> (Updated Dec. 24, 2016, 11:14 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently some tests that use particularly long sandbox paths will due
> to the notorious Windows path length limitations. Here, we change the
> sandbox directory default on Windows agent tests to be something shorter
> than the Unix equivalent in order to avoid these errors.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp a683d8c221635f81906abbead1e01d2469850d93 
> 
> Diff: https://reviews.apache.org/r/55026/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55022: Windows: Cause errors to be correctly reported in `io::read`.

[Alex Clemmer]

> On Jan. 11, 2017, 2:09 a.m., Joseph Wu wrote:
> > The change LGTM, but it might need a rebase due to some changes BenH made 
> > to the same file.  And MPark will also be committing a change to this file 
> > soon (related to `io::peek`).
> > 
> > A few things to note:
> > 
> > * Why not make the same changes to `io::write`?
> > * Remove the TODO that BenH left: `TODO(benh): Confirm that `os::strerror` 
> > does the right thing for `error` on Windows.`
> 
> Joseph Wu wrote:
> Note: the `io::peek` review has been committed, so a rebase will be 
> especially useful now.

I'll do it tonight.


- Alex


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


On Dec. 24, 2016, 9:31 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55022/
> ---
> 
> (Updated Dec. 24, 2016, 9:31 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Cause errors to be correctly reported in `io::read`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 
> 
> Diff: https://reviews.apache.org/r/55022/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55022: Windows: Cause errors to be correctly reported in `io::read`.

[Joseph Wu]

> On Jan. 10, 2017, 6:09 p.m., Joseph Wu wrote:
> > The change LGTM, but it might need a rebase due to some changes BenH made 
> > to the same file.  And MPark will also be committing a change to this file 
> > soon (related to `io::peek`).
> > 
> > A few things to note:
> > 
> > * Why not make the same changes to `io::write`?
> > * Remove the TODO that BenH left: `TODO(benh): Confirm that `os::strerror` 
> > does the right thing for `error` on Windows.`

Note: the `io::peek` review has been committed, so a rebase will be especially 
useful now.


- Joseph


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


On Dec. 24, 2016, 1:31 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55022/
> ---
> 
> (Updated Dec. 24, 2016, 1:31 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Cause errors to be correctly reported in `io::read`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 
> 
> Diff: https://reviews.apache.org/r/55022/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55026: Windows: Shortened test directories on Windows to avoid path errors.

[Joseph Wu]

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


Ship it!




LGTM.

As some background (and for your information), these temporary directories are 
created in tests like `environment->mktemp()`.  This is something we use for 
debugging, as having the test name in the temporary directory makes tracking 
down failed/crashed tests easier.  With this change, it will be even more 
important for tests to be run with `--verbose`; as long as tests are verbose, 
we can track down the appropriate temporary folders for almost all debugging 
purposes.

- Joseph Wu


On Dec. 24, 2016, 3:14 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55026/
> ---
> 
> (Updated Dec. 24, 2016, 3:14 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently some tests that use particularly long sandbox paths will due
> to the notorious Windows path length limitations. Here, we change the
> sandbox directory default on Windows agent tests to be something shorter
> than the Unix equivalent in order to avoid these errors.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp a683d8c221635f81906abbead1e01d2469850d93 
> 
> Diff: https://reviews.apache.org/r/55026/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55410: Used standard classic locale for `jsonify`.

[Michael Park]

> On Jan. 11, 2017, 3:05 a.m., Alexander Rojas wrote:
> > This patch doesn't really fix the issue, the following program fails to 
> > produce appropriate JSON:
> > 
> > ```c++
> > #include 
> > #include 
> > 
> > #include 
> > #include 
> > #include 
> > 
> > int main(int argc, char **argv) {
> >   // Set locale to German.
> >   std::setlocale(LC_ALL, "de_DE.UTF-8");
> >   std::cout.imbue(std::locale("de_DE.UTF-8"));
> > 
> >   std::vector doubles = {1234567890.12345, 123456789.012345};
> >   std::cout << jsonify(doubles) << '\n';
> > 
> >   std::string str = jsonify(doubles);
> >   std::cout << str << '\n';
> >   return 0;
> > }
> > ```
> > 
> > The reason is that the patch changes the locale of the `stream` but 
> > printing is done with `snprintf()`. At the same time, changing the locale 
> > of the stream is not thread safe (although using the stream concurrently 
> > should be heavily discouraged).

Synced with Alexander on Slack, we're going to implement `uselocale` on 
Windows, similar to how libc++ does here: 
https://github.com/llvm-mirror/libcxx/blob/1b34b986bcc7e0991513213c295f3c9c82072a34/src/support/win32/locale_win32.cpp#L26-L39


- Michael


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


On Jan. 11, 2017, 2:03 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55410/
> ---
> 
> (Updated Jan. 11, 2017, 2:03 a.m.)
> 
> 
> Review request for mesos and Alexander Rojas.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `` header doesn't exist in Windows and thus broke
> the Windows build.
> 
> ```
> fatal error C1083: Cannot open include file: 'xlocale.h': No such file
> ```
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 3c48046e087de2a66139a31449327fd94c149371 
> 
> Diff: https://reviews.apache.org/r/55410/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55381, 55271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Jan. 11, 2017, 10:37 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55271/
> ---
> 
> (Updated Jan. 11, 2017, 10:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6631
> https://issues.apache.org/jira/browse/MESOS-6631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently do not allow `MULTI_ROLE` frameworks to change their
> roles. This restriction will be lifted later.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
>   src/tests/master_validation_tests.cpp 
> e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55271/diff/
> 
> 
> Testing
> ---
> 
> Tested on various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55420: Added unit tests for nested container neglected runtime argv.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 11, 2017, 11:53 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55420/
> ---
> 
> (Updated Jan. 11, 2017, 11:53 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, and Jie Yu.
> 
> 
> Bugs: MESOS-6852
> https://issues.apache.org/jira/browse/MESOS-6852
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These are regression tests for MESOS-6852. This patch includes
> the tests for default image entrypoint or cmd with user defined
> arguments appended for nested container case.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> c0c11607024b6a80d5bf5a486b91f7905a9083d7 
> 
> Diff: https://reviews.apache.org/r/55420/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 55419: Changed mesos test helper createCommandInfo() using optional value.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 11, 2017, 11:53 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55419/
> ---
> 
> (Updated Jan. 11, 2017, 11:53 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Avinash sridharan, Artem 
> Harutyunyan, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `value` in protobuf `CommandInfo` is an option. We should allow
> `value` to be empty in mesos test helper createCommandInfo(). The
> use case is the empty value with arguments in runtime isolator
> scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp fa965150ae3e31b4d74dac57148a197402fd551e 
> 
> Diff: https://reviews.apache.org/r/55419/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52612: Added nested container tests for docker runtime isolator.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 11, 2017, 11:50 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52612/
> ---
> 
> (Updated Jan. 11, 2017, 11:50 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Artem 
> Harutyunyan, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6292
> https://issues.apache.org/jira/browse/MESOS-6292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container tests for docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> c0c11607024b6a80d5bf5a486b91f7905a9083d7 
> 
> Diff: https://reviews.apache.org/r/52612/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 55413: Fixed appc runtime isolator argv issue for nested container.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 11, 2017, 10:42 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55413/
> ---
> 
> (Updated Jan. 11, 2017, 10:42 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, and Jie Yu.
> 
> 
> Bugs: MESOS-6852
> https://issues.apache.org/jira/browse/MESOS-6852
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed appc runtime isolator argv issue for nested container.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
> b0782d95107aa030eaadf96f7523b439d44a4b79 
> 
> Diff: https://reviews.apache.org/r/55413/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 55412: Fixed docker runtime isolator argv issue for nested container.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 11, 2017, 10:42 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55412/
> ---
> 
> (Updated Jan. 11, 2017, 10:42 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, and Jie Yu.
> 
> 
> Bugs: MESOS-6852
> https://issues.apache.org/jira/browse/MESOS-6852
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For the nested container with docker runtime isolator, the `argv`
> that should be appended to entrypoint[0] or cmd[0] are neglected.
> This case refers to the 2nd row of the runtime isolator logic
> table (sh=0, value=0, argv=1). This patch addresses the issue.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> ccc2387346699adcfd21886674884c87bce508aa 
> 
> Diff: https://reviews.apache.org/r/55412/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 55241: Stop using os::system to validate perf event names.

[James Peach]

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




src/linux/perf.cpp (line 327)


It adds `true` to the commandline like the original code does. The `perf` 
commands generally need a subcommand to execute.



src/linux/perf.cpp (lines 329 - 341)


I expect that the original author didn't use the `Perf` because they didn't 
want to capture the output. But I don't see why your suggestion wouldn't work.

If it weren't for the redirection I would have just replaced this with 
`spawn`.



src/linux/perf.cpp (line 333)


The original code only redirected `stderr` so I've preserved that here.


- James Peach


On Jan. 6, 2017, 1:17 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55241/
> ---
> 
> (Updated Jan. 6, 2017, 1:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Greg Mann, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to validate perf event names.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 
> 
> Diff: https://reviews.apache.org/r/55241/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55239: Stop using os::system to extract fetcher archives.

[James Peach]

> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > I don't feel that we need to introduce these wrappers, which are either 
> > single-use or don't provide a better abstration than the counterpart in 
> > stout.
> > 
> > Can we uniformly use `subprocess` in all cases (since `os::spawn()` doesn't 
> > work in all cases) for consistency?
> > 
> > To reuse code, we can do this
> > 
> > ```
> > vector command;
> > 
> > // Potentially need to specify destination via stdout.
> > Option out = None();
> > 
> > // if tar.
> > command = {"tar", "-C", destinationDirectory, "-xf", sourcePath};
> > // else if gzip.
> > command = {"gzip", "-dc", sourcePath};
> > out = Subprocess::PATH(destinationPath);
> > // else if zip.
> > command = {"unzip", "-o", "-d", destinationDirectory, sourcePath};
> > // else
> > 
> > ...
> > 
> > // Comment that we use the argv of subprocess to avoid injection.
> > Try s = subprocess(
> > command[0],
> > command,
> > Subprocess::FD(STDIN_FILENO),
> > out.getOrElse(Subprocess::FD(STDOUT_FILENO)));
> > ```
> > 
> > What do you think?

We could subprocess everything. Would need to think about how to format the 
command message legibly.


> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 85
> > 
> >
> > This is the default so unnecessary?

However it makes it obvious to the reader, so it should be kept.


> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 95
> > 
> >
> > `stringify` on `vector` puts `", "` between items.

Yes, that's desirable so that you can see the whitespace in the command.


> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 67
> > 
> >
> > `stringify` on `vector` puts `", "` between items. We can use 
> > `strings::join()`.

This is intended since it makes the actual arguments much clearer and more 
obvious.


- James


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


On Jan. 6, 2017, 1:13 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> ---
> 
> (Updated Jan. 6, 2017, 1:13 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to extract fetcher archives.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
> 
> Diff: https://reviews.apache.org/r/55239/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55239: Stop using os::system to extract fetcher archives.

[James Peach]

> On Jan. 11, 2017, 9:55 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 91
> > 
> >
> > `status()` is not guaranteed to be ready, this could crash the fetcher 
> > process.

This implicitly awaits the future. It would crash if the future failed or is 
discarded, which AFAIK can't happen here.


- James


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


On Jan. 6, 2017, 1:13 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> ---
> 
> (Updated Jan. 6, 2017, 1:13 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to extract fetcher archives.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
> 
> Diff: https://reviews.apache.org/r/55239/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55240: Stop using os::system to copy local files.

[James Peach]

> On Jan. 11, 2017, 9:57 a.m., Jiang Yan Xu wrote:
> > I commented on /r/55239/ about the `_spawn()` helper. We could just use 
> > `os::spawn()` directly here.

The `_spawn` helper is still an improvement here since it gets the error 
message right. If we remove the helper in the previous review, I guess we can 
still use `spawn` here and format the error correctly.


- James


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


On Jan. 6, 2017, 1:15 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55240/
> ---
> 
> (Updated Jan. 6, 2017, 1:15 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to copy local files.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
> 
> Diff: https://reviews.apache.org/r/55240/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55238: Use os::spawn in the CNI isolator.

[James Peach]

> On Jan. 11, 2017, 10:11 a.m., Jiang Yan Xu wrote:
> > I feel if we keep `os::system()` in the codebase at all, this is one of the 
> > few places it could actually be used... we could eliminate it so we can say 
> > there's no references to `os::systems()` left today but it's a bit harsh to 
> > nit-picking on future use like this in order to keep a "clean state"?

I don't consider this harsh, just a minor, obvious improvement. While 
`system()` is safe, `spawn()` is slightly better because it doesn't use the 
shell. We can't completely eliminate `system()` because there are places that 
actually require the shell (eg. the port mapping CNI plugin).


- James


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


On Jan. 6, 2017, 1:12 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55238/
> ---
> 
> (Updated Jan. 6, 2017, 1:12 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use os::spawn in the CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ea91c71fdfac48a2fc1d31a0ee088a73244be367 
> 
> Diff: https://reviews.apache.org/r/55238/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 55420: Added unit tests for nested container neglected runtime argv.

[Gilbert Song]

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

Review request for mesos, Avinash sridharan, Artem Harutyunyan, and Jie Yu.


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


Repository: mesos


Description
---

These are regression tests for MESOS-6852. This patch includes
the tests for default image entrypoint or cmd with user defined
arguments appended for nested container case.


Diffs
-

  src/tests/containerizer/runtime_isolator_tests.cpp 
c0c11607024b6a80d5bf5a486b91f7905a9083d7 

Diff: https://reviews.apache.org/r/55420/diff/


Testing
---

make check


Thanks,

Gilbert Song

Review Request 55419: Changed mesos test helper createCommandInfo() using optional value.

[Gilbert Song]

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

Review request for mesos, Anand Mazumdar, Avinash sridharan, Artem Harutyunyan, 
Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

The `value` in protobuf `CommandInfo` is an option. We should allow
`value` to be empty in mesos test helper createCommandInfo(). The
use case is the empty value with arguments in runtime isolator
scenario.


Diffs
-

  src/tests/mesos.hpp fa965150ae3e31b4d74dac57148a197402fd551e 

Diff: https://reviews.apache.org/r/55419/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 52612: Added nested container tests for docker runtime isolator.

[Gilbert Song]

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

(Updated Jan. 11, 2017, 3:50 a.m.)


Review request for mesos, Anand Mazumdar, Benjamin Hindman, Artem Harutyunyan, 
Jie Yu, Timothy Chen, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added nested container tests for docker runtime isolator.


Diffs (updated)
-

  src/tests/containerizer/runtime_isolator_tests.cpp 
c0c11607024b6a80d5bf5a486b91f7905a9083d7 

Diff: https://reviews.apache.org/r/52612/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 55410: Used standard classic locale for `jsonify`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [55410]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Jan. 11, 2017, 10:03 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55410/
> ---
> 
> (Updated Jan. 11, 2017, 10:03 a.m.)
> 
> 
> Review request for mesos and Alexander Rojas.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `` header doesn't exist in Windows and thus broke
> the Windows build.
> 
> ```
> fatal error C1083: Cannot open include file: 'xlocale.h': No such file
> ```
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 3c48046e087de2a66139a31449327fd94c149371 
> 
> Diff: https://reviews.apache.org/r/55410/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55410: Used standard classic locale for `jsonify`.

[Alexander Rojas]

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



This patch doesn't really fix the issue, the following program fails to produce 
appropriate JSON:

```c++
#include 
#include 

#include 
#include 
#include 

int main(int argc, char **argv) {
  // Set locale to German.
  std::setlocale(LC_ALL, "de_DE.UTF-8");
  std::cout.imbue(std::locale("de_DE.UTF-8"));

  std::vector doubles = {1234567890.12345, 123456789.012345};
  std::cout << jsonify(doubles) << '\n';

  std::string str = jsonify(doubles);
  std::cout << str << '\n';
  return 0;
}
```

The reason is that the patch changes the locale of the `stream` but printing is 
done with `snprintf()`. At the same time, changing the locale of the stream is 
not thread safe (although using the stream concurrently should be heavily 
discouraged).

- Alexander Rojas


On Jan. 11, 2017, 11:03 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55410/
> ---
> 
> (Updated Jan. 11, 2017, 11:03 a.m.)
> 
> 
> Review request for mesos and Alexander Rojas.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `` header doesn't exist in Windows and thus broke
> the Windows build.
> 
> ```
> fatal error C1083: Cannot open include file: 'xlocale.h': No such file
> ```
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 3c48046e087de2a66139a31449327fd94c149371 
> 
> Diff: https://reviews.apache.org/r/55410/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 55413: Fixed appc runtime isolator argv issue for nested container.

[Gilbert Song]

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

Review request for mesos, Avinash sridharan, Artem Harutyunyan, and Jie Yu.


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


Repository: mesos


Description
---

Fixed appc runtime isolator argv issue for nested container.


Diffs
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
b0782d95107aa030eaadf96f7523b439d44a4b79 

Diff: https://reviews.apache.org/r/55413/diff/


Testing
---

make check


Thanks,

Gilbert Song

Review Request 55412: Fixed docker runtime isolator argv issue for nested container.

[Gilbert Song]

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

Review request for mesos, Avinash sridharan, Artem Harutyunyan, and Jie Yu.


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


Repository: mesos


Description
---

For the nested container with docker runtime isolator, the `argv`
that should be appended to entrypoint[0] or cmd[0] are neglected.
This case refers to the 2nd row of the runtime isolator logic
table (sh=0, value=0, argv=1). This patch addresses the issue.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
ccc2387346699adcfd21886674884c87bce508aa 

Diff: https://reviews.apache.org/r/55412/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 55381: Added test for framework upgrading to multi-role capability.

[Benjamin Bannier]

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

(Updated Jan. 11, 2017, 11:37 a.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/master_validation_tests.cpp 
e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55381/diff/


Testing
---

make check (OS X)


Thanks,

Benjamin Bannier

Re: Review Request 55271: Disallow multi-role frameworks to change their roles.

[Benjamin Bannier]

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

(Updated Jan. 11, 2017, 11:37 a.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
---

Rebased, added proper dep on r/55381.


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


Repository: mesos


Description
---

We currently do not allow `MULTI_ROLE` frameworks to change their
roles. This restriction will be lifted later.


Diffs (updated)
-

  src/master/master.cpp 1746a88953dbdc148d98881bcf7027b62ad6b040 
  src/tests/master_validation_tests.cpp 
e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 

Diff: https://reviews.apache.org/r/55271/diff/


Testing
---

Tested on various Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 55238: Use os::spawn in the CNI isolator.

[Jiang Yan Xu]

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



I feel if we keep `os::system()` in the codebase at all, this is one of the few 
places it could actually be used... we could eliminate it so we can say there's 
no references to `os::systems()` left today but it's a bit harsh to nit-picking 
on future use like this in order to keep a "clean state"?

- Jiang Yan Xu


On Jan. 5, 2017, 5:12 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55238/
> ---
> 
> (Updated Jan. 5, 2017, 5:12 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use os::spawn in the CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ea91c71fdfac48a2fc1d31a0ee088a73244be367 
> 
> Diff: https://reviews.apache.org/r/55238/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55242: Stop using os::system to chown a directory hierarchy.

[Jiang Yan Xu]

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



If we implement the recursive chown this way, I feel that we need some unit 
tests, I don't think higher-level tests have enough coverage for this method. 
If we replace it with `os::spawn()` it's more trivial that I think we can skip 
the test for now. What do you prefer?

- Jiang Yan Xu


On Jan. 5, 2017, 5:18 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55242/
> ---
> 
> (Updated Jan. 5, 2017, 5:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to chown a directory hierarchy.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/chown.hpp 
> c82e2e574019c5ee5f17ea105a6d225006388a45 
> 
> Diff: https://reviews.apache.org/r/55242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 55410: Used standard classic locale for `jsonify`.

[Michael Park]

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

Review request for mesos and Alexander Rojas.


Repository: mesos


Description
---

The `` header doesn't exist in Windows and thus broke
the Windows build.

```
fatal error C1083: Cannot open include file: 'xlocale.h': No such file
```


Diffs
-

  3rdparty/stout/include/stout/jsonify.hpp 
3c48046e087de2a66139a31449327fd94c149371 

Diff: https://reviews.apache.org/r/55410/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 55240: Stop using os::system to copy local files.

[Jiang Yan Xu]

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



I commented on /r/55239/ about the `_spawn()` helper. We could just use 
`os::spawn()` directly here.

- Jiang Yan Xu


On Jan. 5, 2017, 5:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55240/
> ---
> 
> (Updated Jan. 5, 2017, 5:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to copy local files.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
> 
> Diff: https://reviews.apache.org/r/55240/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55239: Stop using os::system to extract fetcher archives.

[Jiang Yan Xu]

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




src/launcher/fetcher.cpp (line 91)


`status()` is not guaranteed to be ready, this could crash the fetcher 
process.


- Jiang Yan Xu


On Jan. 5, 2017, 5:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> ---
> 
> (Updated Jan. 5, 2017, 5:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to extract fetcher archives.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
> 
> Diff: https://reviews.apache.org/r/55239/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55241: Stop using os::system to validate perf event names.

[Jiang Yan Xu]

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



It's worth noting that the perf events are specified via agent flags so no 
unprivileged / remote user input is involved but I guess the goal is to 
eliminate all instances of `os::system()` (at least the ones with any 
non-static commands)? Are the reviews in this JIRA combined achieving that?


src/linux/perf.cpp (line 320)


s/{ "perf", "stat", "--log-fd", "2" }/{"perf", "stat", "--log-fd", "2"}/



src/linux/perf.cpp (line 327)


What does `true` do in here?



src/linux/perf.cpp (lines 329 - 341)


I am not sure how much I like the `internal::Perf` abtraction but it's 
there and other similar calls in this file use it so we might as well just do:

```
internal::Perf* perf = new internal::Perf(argv);
Future output = perf->output();
spawn(perf, true);
output.await();

// The output being ready means the the command is successful and the 
events are valid.
return output.isReady();
```



src/linux/perf.cpp (line 333)


I guess we don't need to see stdout?


- Jiang Yan Xu


On Jan. 5, 2017, 5:17 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55241/
> ---
> 
> (Updated Jan. 5, 2017, 5:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Greg Mann, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to validate perf event names.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp aa31982eb5358b7eafa7035f4358a88d3854755f 
> 
> Diff: https://reviews.apache.org/r/55241/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55239: Stop using os::system to extract fetcher archives.

[Jiang Yan Xu]

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



I don't feel that we need to introduce these wrappers, which are either 
single-use or don't provide a better abstration than the counterpart in stout.

Can we uniformly use `subprocess` in all cases (since `os::spawn()` doesn't 
work in all cases) for consistency?

To reuse code, we can do this

```
vector command;

// Potentially need to specify destination via stdout.
Option out = None();

// if tar.
command = {"tar", "-C", destinationDirectory, "-xf", sourcePath};
// else if gzip.
command = {"gzip", "-dc", sourcePath};
out = Subprocess::PATH(destinationPath);
// else if zip.
command = {"unzip", "-o", "-d", destinationDirectory, sourcePath};
// else

...

// Comment that we use the argv of subprocess to avoid injection.
Try s = subprocess(
command[0],
command,
Subprocess::FD(STDIN_FILENO),
out.getOrElse(Subprocess::FD(STDOUT_FILENO)));
```

What do you think?


src/launcher/fetcher.cpp (line 67)


`stringify` on `vector` puts `", "` between items. We can use 
`strings::join()`.



src/launcher/fetcher.cpp (line 85)


This is the default so unnecessary?



src/launcher/fetcher.cpp (line 95)


`stringify` on `vector` puts `", "` between items.


- Jiang Yan Xu


On Jan. 5, 2017, 5:13 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> ---
> 
> (Updated Jan. 5, 2017, 5:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stop using os::system to extract fetcher archives.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f 
> 
> Diff: https://reviews.apache.org/r/55239/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.

[Alex Clemmer]

> On Jan. 11, 2017, 2:19 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1046
> > 
> >
> > s/teard down/teardown/
> > 
> > `process::finalize` should probably perform the socket teardown, or at 
> > least give the option to do so.  
> > 
> > Currently, very few of our processes call `process::finalize`.  They 
> > usually just rely on the OS cleaning up after them.

I'm pretty comfortable allowing `process::finalize` to clean up after itself, 
especially resources that it owns and abstracts away, but disposing of 
something that libprocess _doesn't_ own, and especially something as global and 
critical as the socket stack, seems a bit out of scope.

I do think it's prudent to entertain proposals about having it _optionally_ 
dispose of the socket stack, but I can't think of an obviously good way to do 
this. A default parameter in `process::finalize` would only be meaningful 
semantically on Windows, no?


> On Jan. 11, 2017, 2:19 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 1040-1041
> > 
> >
> > Even if this is idempotent, we can remove the call from some of our own 
> > binaries now.
> > 
> > Also, what is the version check you mention here?

I'll clarify the comment. What I was saying is that `wsa_initialize` will turn 
on the socket stack (which may be only on, which is fine because it's 
idempotent), but also check that its version is compatible with Stout and 
libprocess.


- Alex


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


On Dec. 24, 2016, 10:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55024/
> ---
> 
> (Updated Dec. 24, 2016, 10:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently libprocess will attempt to use sockets without initializing
> the socket stack on Windows. This commit will resolving this problem by
> causing `process::initialize` to initialize the socket stack.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 889a03444eaee7b5ad2be65bb414c30062d4a4f0 
> 
> Diff: https://reviews.apache.org/r/55024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>