Re: Review Request 67441: Testing windows reviewbot utf8 handling.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67441]

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

- Mesos Reviewbot


On June 4, 2018, 3:34 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67441/
> ---
> 
> (Updated June 4, 2018, 3:34 p.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Testing windows reviewbot utf8 handling.
> 
> 
> Diffs
> -
> 
>   foo PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67441/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 67441: Testing windows reviewbot utf8 handling.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67441 was successfully built and tested.

Reviews applied: `['67441']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67441

- Mesos Reviewbot Windows


On June 4, 2018, 10:34 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67441/
> ---
> 
> (Updated June 4, 2018, 10:34 p.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Testing windows reviewbot utf8 handling.
> 
> 
> Diffs
> -
> 
>   foo PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67441/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 67388: Moved: `io::internal::read/write` to separate file.

[Joseph Wu]

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



General note on code organization:

Since you are splitting the `io::internal::read/write` functions into Posix and 
Windows implementations, it would be helpful to organize the code like:
```
3rdparty/libprocess/src/
|-- io/
|-- io.cpp
|
|-- posix/
|   |-- io.hpp // Instead of io_internal.hpp
|   |-- io.cpp // Instead of poll_io.cpp
|
|-- windows/
|   // Instead of libwinio_impl.hpp ( 
https://reviews.apache.org/r/67389/ )
|   // Possibly combine libwinio.hpp too ( 
https://reviews.apache.org/r/67390/ )
|-- event_loop.hpp 
|
|   // Instead of libwinio_impl.cpp ( 
https://reviews.apache.org/r/67389/ )
|   // Possibly combine libwinio_eventloop.cpp too ( 
https://reviews.apache.org/r/67390/ )
|-- event_loop.cpp 
|
|-- io.hpp 
|-- io.cpp // Instead of libwinio_io.cpp ( 
https://reviews.apache.org/r/67390/ )
|-- ...
```

- Joseph Wu


On May 30, 2018, 11:55 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67388/
> ---
> 
> (Updated May 30, 2018, 11:55 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation in `io::internal::read/write` uses `io::poll`, which
> is UNIX specific. The Windows IOCP implementation will not use a
> polling function, since there no unified mechanism, so the functions
> have been moved to their own file.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819 
>   3rdparty/libprocess/src/CMakeLists.txt 
> cf443dffd0663ecf02b7efd6f7094175b94aae19 
>   3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba 
>   3rdparty/libprocess/src/io_internal.hpp PRE-CREATION 
>   3rdparty/libprocess/src/poll_io.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67388/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67157: Refactored the gRPC client runtime wrapper in libprocess.

[Chun-Hung Hsiao]

> On May 25, 2018, 3:12 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/grpc.cpp
> > Lines 76 (patched)
> > 
> >
> > This seems almost like the libprocess equivalent of a throwing 
> > destructor. Could we trigger this e.g., if we fail a test `ASSERT`?
> > 
> > Also see my comment regarding joining whenever `looper` goes out of 
> > scope.

Given that we never directly call `process::terminate` on a `RuntimeProcess`, 
but only indirectly terminate the process through `RuntimeProcess::terminate` 
which also sets `terminating`, this cannot fail at any case, including failed 
test assertions. I could remove this if this line looks weird to you.


- Chun-Hung


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


On May 17, 2018, 10:23 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> ---
> 
> (Updated May 17, 2018, 10:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
>process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
>managing its lifecycle in `Runtime::Data`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/grpc.hpp 
> 321a46e19c69eafb24012bcef68bb8b0cc6aa436 
>   3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0 
> 
> 
> Diff: https://reviews.apache.org/r/67157/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

[Jie Yu]

> On June 4, 2018, 11:13 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/nested_mesos_containerizer_tests.cpp
> > Line 461 (original), 486 (patched)
> > 
> >
> > wget is not usually installed by default...

That's true. One possibility is to use named pipe
https://linux.die.net/man/3/mkfifo


- Jie


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


On June 1, 2018, 3:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 1, 2018, 3:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

[Jie Yu]

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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 583-585 (original), 584-586 (patched)


I think there's a race here. If the process is spawned, but the initailize 
is not called yet, the command might return a failure?

I think a better way is to have a helper in the Waiter so that we can wait 
until the route has been setup.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Line 1435 (original), 1426-1427 (patched)


Hum, I think if the route is not setup, the wget will get a 404?

My suggestion is to make this more explicit. For instance, have a rourte 
called `/trigger` and a Promise called `triggered` (for client to trigger an 
event that test is waiting), and have another route called `/wait` and have a 
Promise called `notify` (for test to notify the client).


- Jie Yu


On June 1, 2018, 3:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 1, 2018, 3:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

[Andrew Schwartzmeyer]

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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Line 461 (original), 486 (patched)


wget is not usually installed by default...


- Andrew Schwartzmeyer


On June 1, 2018, 8:55 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 1, 2018, 8:55 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67064: Added libarchive, bzip2, and xz patches and associated build changes.

[Joseph Wu]

> On June 4, 2018, 1:50 p.m., Joseph Wu wrote:
> > src/Makefile.am
> > Lines 218 (patched)
> > 
> >
> > Won't need this line when linking against the bundled libarchive since 
> > we'll be adding the library to libmesos further down the makefile.

Ah, nevermind.  This becomes necessary as the build rules for libmesos and 
mesos-fetcher are slightly separate in the automake build.


- Joseph


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


On June 4, 2018, 12:11 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67064/
> ---
> 
> (Updated June 4, 2018, 12:11 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These libraries being available will allow stout to be changed such that
> stout can invoke libarchive instead of shelling out to tar to extract
> archives. On Windows, tar usually doesn't exist, and even if it does, it
> rarely supports most compression formats. On Windows, we will build
> libarchive to support each of those compression formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt ecb6946401d9b81c6610cf9f33dcf2caa9ff0f04 
>   3rdparty/Makefile.am 062df18643cadb4f8f5bcbd56c1dad63a7b8f894 
>   3rdparty/bzip2-1.0.6.patch PRE-CREATION 
>   3rdparty/cmake/Versions.cmake 2828acd2aa00c8778c6c462bc594716b2b6289ba 
>   3rdparty/libarchive-3.3.2.patch PRE-CREATION 
>   3rdparty/versions.am ada998d62d012a45b2cf17256c1ae16b92d611f2 
>   3rdparty/xz-5.2.3.patch PRE-CREATION 
>   configure.ac 03d333d65bcab2e46cff0b1329e5ad7bb26da697 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
> 
> 
> Diff: https://reviews.apache.org/r/67064/diff/8/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Review Request 67441: Testing windows reviewbot utf8 handling.

[Gastón Kleiman]

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

Review request for mesos and Andrew Schwartzmeyer.


Repository: mesos


Description
---

Testing windows reviewbot utf8 handling.


Diffs
-

  foo PRE-CREATION 


Diff: https://reviews.apache.org/r/67441/diff/1/


Testing
---


Thanks,

Gastón Kleiman

Re: Review Request 67425: [WIP] Implement a JWK Set parser converting JWKs into RSA keys.

[Clement Michaud]

> On juin 4, 2018, 2:04 après-midi, Alexander Rojas wrote:
> > Before going into detail with this, last patchset we forgot to add certain 
> > files to the install target wich ended up breaking tarball generation. 
> > Mostly a failure of your reviewers (myself included). Since you are adding 
> > new files, could you please make sure it works with `make distcheck` and 
> > also make sure of updating the `cmake` build (that should be a rule of 
> > thumb whenever one modifies the `Makefile.am`).

Yes, sure. I did not know I had to run those commands and did not get any 
failure when not doing so. I will make sure to run them from now on. Sorry for 
that.
It would be good to add it in the test workflow though, don't you think?

Thanks.
Clément.


- Clement


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


On juin 4, 2018, 1:27 après-midi, Clement Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> ---
> 
> (Updated juin 4, 2018, 1:27 après-midi)
> 
> 
> Review request for mesos and Alexander Rojas.
> 
> 
> Bugs: MESOS-8974
> https://issues.apache.org/jira/browse/MESOS-8974
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
> 
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
> 
> The parser only supports parsing of  RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819 
>   3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION 
>   3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67425/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Clement Michaud
> 
>

Re: Review Request 67066: Modified the fetcher to use libarchive and added associated tests.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On June 4, 2018, 12:11 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67066/
> ---
> 
> (Updated June 4, 2018, 12:11 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the fetcher to use libarchive and added associated tests.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4cff7273fdc9e3897074da4da7dad02483086c2d 
>   src/tests/fetcher_tests.cpp 8c353f2a30a6dd6338c91dad7ff76ff28d1005e8 
> 
> 
> Diff: https://reviews.apache.org/r/67066/diff/9/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

[Joseph Wu]

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


Ship it!




A bunch of minor things that I'll fix before committing.


3rdparty/stout/include/stout/archiver.hpp
Lines 71-76 (patched)


Two newlines too many here.  I can fixup before committing.



3rdparty/stout/tests/CMakeLists.txt
Lines 106 (patched)


Extraneous newline which seems to have snuck into the review.  I'll remove 
before committing.



3rdparty/stout/tests/archiver_tests.cpp
Lines 42 (patched)


s/os::getcwd()/sandbox.get()/

Here and throughout the test.  The TemporaryDirectoryTest defines this 
variable so that we don't need to make that call repeatedly.



3rdparty/stout/tests/archiver_tests.cpp
Lines 52 (patched)


A few periods missing on comments like this throughout this file.



3rdparty/stout/tests/archiver_tests.cpp
Lines 94 (patched)


s/fo/to/



3rdparty/stout/tests/archiver_tests.cpp
Lines 411 (patched)


s/extrat/extract/



3rdparty/stout/tests/archiver_tests.cpp
Lines 457-458 (patched)


This test passes on non-Windows too, so it's better not to `#ifdef` it.


- Joseph Wu


On June 4, 2018, 12:11 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated June 4, 2018, 12:11 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/8/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67064: Added libarchive, bzip2, and xz patches and associated build changes.

[Joseph Wu]

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


Fix it, then Ship it!




A couple of small things I'll fix before committing:


3rdparty/CMakeLists.txt
Lines 937-948 (patched)


We should add `-DENABLE_LibGCC=OFF` just to be safe.

The option could have an effect on Windows if the condition `NOT 
WITHOUT_PCRE_STATIC AND NOT PCRE_STATIC AND PCRE_FOUND` is satsified.  AFAICT, 
none of these variables are usually defined.



3rdparty/CMakeLists.txt
Lines 954 (patched)


Hum... tests appear to be disabled on all platforms...

I'll change this to be a note about the linkage against other compression 
libraries (bzip2, xz, zlib).



3rdparty/Makefile.am
Line 234 (original), 237 (patched)


Some extraneous spacing changes (mostly tabs to spaces) seem to have snuck 
into this patch.  I'll remove them before committing.



3rdparty/Makefile.am
Lines 281-290 (patched)


We'll need to disable building bzip2 and xz via:
```
--without-bz2lib
--without-lzma
```
As these are not explicit dependencies.  The libarchive build attempts to 
find these on the system, and if found, it will require symbols found in those 
libraries.  We'd then get a link-time failure as Mesos does not link to `-lbz2` 
and `-lxz`.



3rdparty/libarchive-3.3.2.patch
Lines 32-39 (patched)


The whitespace changes in the patch are extraneous and can be removed.  (We 
don't need to prettify 3rdparty sources :)



configure.ac
Lines 1469 (patched)


Missing a part of the conditional here:
`|| test "x$enable_bundled" != "xyes"`

I can add this before committing.



src/Makefile.am
Lines 218 (patched)


Won't need this line when linking against the bundled libarchive since 
we'll be adding the library to libmesos further down the makefile.



3rdparty/CMakeLists.txt
Lines 987 (patched)


I almost didn't notice the odd capitalization on these:
`SET_target_properties`
vs
`set_target_properties`

I'll fix up the three instances of this before committing.


- Joseph Wu


On June 4, 2018, 12:11 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67064/
> ---
> 
> (Updated June 4, 2018, 12:11 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These libraries being available will allow stout to be changed such that
> stout can invoke libarchive instead of shelling out to tar to extract
> archives. On Windows, tar usually doesn't exist, and even if it does, it
> rarely supports most compression formats. On Windows, we will build
> libarchive to support each of those compression formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt ecb6946401d9b81c6610cf9f33dcf2caa9ff0f04 
>   3rdparty/Makefile.am 062df18643cadb4f8f5bcbd56c1dad63a7b8f894 
>   3rdparty/bzip2-1.0.6.patch PRE-CREATION 
>   3rdparty/cmake/Versions.cmake 2828acd2aa00c8778c6c462bc594716b2b6289ba 
>   3rdparty/libarchive-3.3.2.patch PRE-CREATION 
>   3rdparty/versions.am ada998d62d012a45b2cf17256c1ae16b92d611f2 
>   3rdparty/xz-5.2.3.patch PRE-CREATION 
>   configure.ac 03d333d65bcab2e46cff0b1329e5ad7bb26da697 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
> 
> 
> Diff: https://reviews.apache.org/r/67064/diff/8/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67118: Added bzip2, libarchive, and xz tarballs.

[Joseph Wu]

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


Ship it!




The xz.README could use a little bit more explaination.  I'll add some more 
before committing:

```
The xz archive is modified to contain only the minimal files required
to build on Windows.  The original archive contains several files with
licenses that are incompatible with with the Apache License,
and therefore, cannot be bundled into the project.
```

- Joseph Wu


On May 24, 2018, 7:08 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67118/
> ---
> 
> (Updated May 24, 2018, 7:08 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added bzip2, libarchive, and xz tarballs.
> 
> 
> Diffs
> -
> 
>   3rdparty/bzip2-1.0.6.tar.gz PRE-CREATION 
>   3rdparty/libarchive-3.3.2.tar.gz PRE-CREATION 
>   3rdparty/xz-5.2.3-modified.tar.gz PRE-CREATION 
>   3rdparty/xz.README.txt PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67118/diff/2/
> 
> 
> Testing
> ---
> 
> This is just the tarballs for each added archive, no testing required.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67066: Modified the fetcher to use libarchive and added associated tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67066 was successfully built and tested.

Reviews applied: `['67118', '67064', '67065', '67066']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67066

- Mesos Reviewbot Windows


On June 4, 2018, 7:11 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67066/
> ---
> 
> (Updated June 4, 2018, 7:11 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the fetcher to use libarchive and added associated tests.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4cff7273fdc9e3897074da4da7dad02483086c2d 
>   src/tests/fetcher_tests.cpp 8c353f2a30a6dd6338c91dad7ff76ff28d1005e8 
> 
> 
> Diff: https://reviews.apache.org/r/67066/diff/9/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67374: Updated flags to support choosing the random sorter.

[Benjamin Mahler]

> On June 1, 2018, 8:53 p.m., Meng Zhu wrote:
> > include/mesos/allocator/allocator.hpp
> > Line 65 (original), 68 (patched)
> > 
> >
> > I think exporting quotaRole sorter could also be useful, add a TODO?

Yeah, although I think if we were to do this we probably would need the quota 
guarantee sorter to be configurable at framework level?


- Benjamin


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


On May 30, 2018, 1:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67374/
> ---
> 
> (Updated May 30, 2018, 1:11 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-8936
> https://issues.apache.org/jira/browse/MESOS-8936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This deprecates `--user_sorter` in favor of `--role_sorter`.
> 
> This patch also modifies the `--allocator` default to exclude "DRF"
> from the value, while still ensuring the previous default works.
> 
> For now, both sorter flags must be set to `drf` or `random`.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> e5a5355646c834280008867e89eb258eaefc2f1d 
>   src/master/allocator/allocator.cpp e3dd7376b5f05e72d9b9aa895ed3aebc461e4372 
>   src/master/constants.hpp b31e5f027b44d371050cea9a4662a4b54127812f 
>   src/master/flags.hpp 4715f2bf4eb62808265f3062f93e370faa53e63d 
>   src/master/flags.cpp 4ad13752719daf2cfad1f578edffc1f16a101760 
>   src/master/main.cpp 3eb1f89cb877fdd84434954ed80e753b1f859338 
> 
> 
> Diff: https://reviews.apache.org/r/67374/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67372: Updated sorter tests to run against multiple sorters where possible.

[Benjamin Mahler]

> On June 1, 2018, 9:37 p.m., Meng Zhu wrote:
> > src/tests/sorter_tests.cpp
> > Lines 1622-1623 (original), 1620-1621 (patched)
> > 
> >
> > Why are you changing the original setup and only benchmarking one value?

I probably shouldn't be tweaking this in this patch instead of its own patch. :)

There were previously 36 testing parameters which took a long time to run, but 
in order to run them to completion I had to reduce the number of 
configurations, so I figured I would just include the max number of agents and 
clients, and if someone wants to run them with fewer they can update the code.

I'll update this patch to avoid changing the values, we can follow up with 
tweaking them.


- Benjamin


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


On May 30, 2018, 1:02 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67372/
> ---
> 
> (Updated May 30, 2018, 1:02 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-8936
> https://issues.apache.org/jira/browse/MESOS-8936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Several of the sorter tests, including the benchmarks, were agnostic
> to the sorter implementation. These have been updated to run for both
> `DRFSorter` and `RandomSorter`. The remainder have been updated to
> be called DRFSorter tests.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp da4e0f64a565af1d9458ff256ae0eafddd0a6b68 
> 
> 
> Diff: https://reviews.apache.org/r/67372/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67066: Modified the fetcher to use libarchive and added associated tests.

[John Kordich via Review Board]

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

(Updated June 4, 2018, 7:11 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.


Changes
---

Updating review for new reviewbot pass


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


Repository: mesos


Description
---

Modified the fetcher to use libarchive and added associated tests.


Diffs (updated)
-

  src/launcher/fetcher.cpp 4cff7273fdc9e3897074da4da7dad02483086c2d 
  src/tests/fetcher_tests.cpp 8c353f2a30a6dd6338c91dad7ff76ff28d1005e8 


Diff: https://reviews.apache.org/r/67066/diff/9/

Changes: https://reviews.apache.org/r/67066/diff/8-9/


Testing
---

I've built on Windows with CMake and have run all tests, which all pass.
I've also built on Linux with both the autotools build and cmake build and have 
run all tests, which all pass.


Thanks,

John Kordich

Re: Review Request 67064: Added libarchive, bzip2, and xz patches and associated build changes.

[John Kordich via Review Board]

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

(Updated June 4, 2018, 7:11 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.


Changes
---

Addressed Andy's comments


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


Repository: mesos


Description
---

These libraries being available will allow stout to be changed such that
stout can invoke libarchive instead of shelling out to tar to extract
archives. On Windows, tar usually doesn't exist, and even if it does, it
rarely supports most compression formats. On Windows, we will build
libarchive to support each of those compression formats.


Diffs (updated)
-

  3rdparty/CMakeLists.txt ecb6946401d9b81c6610cf9f33dcf2caa9ff0f04 
  3rdparty/Makefile.am 062df18643cadb4f8f5bcbd56c1dad63a7b8f894 
  3rdparty/bzip2-1.0.6.patch PRE-CREATION 
  3rdparty/cmake/Versions.cmake 2828acd2aa00c8778c6c462bc594716b2b6289ba 
  3rdparty/libarchive-3.3.2.patch PRE-CREATION 
  3rdparty/versions.am ada998d62d012a45b2cf17256c1ae16b92d611f2 
  3rdparty/xz-5.2.3.patch PRE-CREATION 
  configure.ac 03d333d65bcab2e46cff0b1329e5ad7bb26da697 
  src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 


Diff: https://reviews.apache.org/r/67064/diff/8/

Changes: https://reviews.apache.org/r/67064/diff/7-8/


Testing
---

I've built on Windows with CMake and have run all tests, which all pass.
I've also built on Linux with both the autotools build and cmake build and have 
run all tests, which all pass.


Thanks,

John Kordich

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

[John Kordich via Review Board]

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

(Updated June 4, 2018, 7:11 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.


Changes
---

Addressed Andy's comments


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


Repository: mesos


Description
---

This archiver utility can be invoked to extract a compressed or
uncompressed archive with several different supported formats.


Diffs (updated)
-

  3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
  3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
  3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
  3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 86111a8709cab709ba1bf844043d3a1da07b6241 
  3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67065/diff/8/

Changes: https://reviews.apache.org/r/67065/diff/7-8/


Testing
---

I've built on Windows with CMake and have run all tests, which all pass.
I've also built on Linux with both the autotools build and cmake build and have 
run all tests, which all pass.


Thanks,

John Kordich

Re: Review Request 67371: Introduced a random sorter as an alternative to the DRF sorter.

[Benjamin Mahler]

> On June 1, 2018, 12:26 a.m., Meng Zhu wrote:
> >

Great review, thanks for catching all of these!


- Benjamin


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


On May 30, 2018, 1 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67371/
> ---
> 
> (Updated May 30, 2018, 1 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-8936
> https://issues.apache.org/jira/browse/MESOS-8936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sorter returns a weighted random shuffling of its clients
> upon each `sort()` request.
> 
> This implementation is a copy of the `DRFSorter` with share
> calculation logic (including the `dirty` bit) removed and an
> adjusted `sort()` implementation. Work needs to be done to
> reduce the amount of duplicated logic needed across sorter
> implementations, but it is left unaddressed here.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/master/allocator/sorter/random/sorter.hpp PRE-CREATION 
>   src/master/allocator/sorter/random/sorter.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67371/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests added in subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67395: Fixed socket creation bug in docker.cpp.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On May 30, 2018, 7:59 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67395/
> ---
> 
> (Updated May 30, 2018, 7:59 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, the statement `int_fd socket = ::socket(...);` would
> implictly call the `WindowsFD(int crt)` constructor. Since that
> contstructor only accepts values of {0, 1, 2}, it would incorrectly
> mark the socket as invalid. The code has been changed to use the stout
> network functions, which properly construct the `int_fd`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp fc032367400119dc827657d0e6e859d18ebdbb16 
> 
> 
> Diff: https://reviews.apache.org/r/67395/diff/1/
> 
> 
> Testing
> ---
> 
> This bug was found when testing recovery in our Windows cluster and this 
> patch was confirmed to fix it.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On June 1, 2018, 7:17 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated June 1, 2018, 7:17 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.

[Joseph Wu]

> On May 31, 2018, 9:41 a.m., Vinod Kone wrote:
> > Can you add a unit test for this?
> 
> Benno Evers wrote:
> It's tricky because we need very precise control over the scheduling, and 
> I'm not sure our testing infrastructure provides it. But I'll look into it.
> 
> Vinod Kone wrote:
> I see.  The bug is in the allocator, so you cannot use a mock allocator 
> unfortunately for control. Consider pausing the clock to have better control 
> in the test.
> 
> Benno Evers wrote:
> After discussing with Benjamin Bannier, we came to the conclusion that 
> it's currently not possible to write a unit test for this scenario, because 
> we're lacking the capability to intercept a dispatch and re-insert it into 
> the event queue at a later time.
> 
> Joseph Wu wrote:
> I gave writing the test a shot... and I think it might be possible, but 
> the resulting test would be too fragile to be a regression test.
> 
> Here's my (not working yet) attempt: 
> https://github.com/kaysoky/mesos/commit/29c6a1807d65d01440b7c67a73062ae9af892afe
> 
> Benno Evers wrote:
> Do you plan to continue working on that, or should we go ahead and commit 
> the fix?

I'll commit this patch shortly.

The test is more of an experiment to see how bad a test for this scenario would 
look like :D


- Joseph


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


On June 1, 2018, 7:17 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated June 1, 2018, 7:17 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

[Andrew Schwartzmeyer]

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




3rdparty/stout/include/stout/archiver.hpp
Lines 82 (patched)


Super tiny style nit (but this will fail mesos-tidy): `fd->crt()`.



3rdparty/stout/include/stout/archiver.hpp
Lines 84 (patched)


s/crt/CRT

But also this needs a comment similar to:

NOTE: On Windows, we need to explicitly allocate a CRT file descriptor 
because libarchive requires it. Once the CRT `fd` is allocated, it must be 
closed with `_close` instead of `os::close`.



3rdparty/stout/include/stout/archiver.hpp
Lines 94-98 (patched)


This `Closer` could be shared, which the ifdef just in the dtor.



3rdparty/stout/include/stout/archiver.hpp
Lines 101 (patched)


Magic number?



3rdparty/stout/include/stout/archiver.hpp
Lines 165-167 (patched)


Error handling looks way better than before :)


- Andrew Schwartzmeyer


On June 1, 2018, 3:40 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated June 1, 2018, 3:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/7/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

[Andrew Schwartzmeyer]

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




3rdparty/stout/include/stout/archiver.hpp
Lines 20-22 (patched)


I still don't think this is necessary here.

Joe had said in an issue:

> This definition should go into the CMake file(s):
> # This is a definition that only has an effect on Windows and is necessary
> # when building a static libarchive instead of a DLL.
> `target_compile_definitions(libarchive PUBLIC LIBARCHIVE_STATIC)`

And then you said in a new iteration:

> Updated to include a comment about why we need a define. There were plans 
to set this define at a global level inside the cmake build using the 
target_compile_definitions cmake function, but this function is available for 
IMPORTED targets only in cmake versions > 3.11. We don't want to bump the 
required version of cmake for such a small change, so we kept the #define in 
this file.

But that's because for imported targets, you're "supposed" to set 
properties manually. So this should instead look like:

```
set_target_properties(
  archive PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS LIBARCHIVE_STATIC)
```


- Andrew Schwartzmeyer


On June 1, 2018, 3:40 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated June 1, 2018, 3:40 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/7/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.

[Benno Evers]

> On May 31, 2018, 4:41 p.m., Vinod Kone wrote:
> > Can you add a unit test for this?
> 
> Benno Evers wrote:
> It's tricky because we need very precise control over the scheduling, and 
> I'm not sure our testing infrastructure provides it. But I'll look into it.
> 
> Vinod Kone wrote:
> I see.  The bug is in the allocator, so you cannot use a mock allocator 
> unfortunately for control. Consider pausing the clock to have better control 
> in the test.
> 
> Benno Evers wrote:
> After discussing with Benjamin Bannier, we came to the conclusion that 
> it's currently not possible to write a unit test for this scenario, because 
> we're lacking the capability to intercept a dispatch and re-insert it into 
> the event queue at a later time.
> 
> Joseph Wu wrote:
> I gave writing the test a shot... and I think it might be possible, but 
> the resulting test would be too fragile to be a regression test.
> 
> Here's my (not working yet) attempt: 
> https://github.com/kaysoky/mesos/commit/29c6a1807d65d01440b7c67a73062ae9af892afe

Do you plan to continue working on that, or should we go ahead and commit the 
fix?


- Benno


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


On June 1, 2018, 2:17 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated June 1, 2018, 2:17 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67425: [WIP] Implement a JWK Set parser converting JWKs into RSA keys.

[Alexander Rojas]

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



Before going into detail with this, last patchset we forgot to add certain 
files to the install target wich ended up breaking tarball generation. Mostly a 
failure of your reviewers (myself included). Since you are adding new files, 
could you please make sure it works with `make distcheck` and also make sure of 
updating the `cmake` build (that should be a rule of thumb whenever one 
modifies the `Makefile.am`).

- Alexander Rojas


On June 4, 2018, 3:27 p.m., Clement Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> ---
> 
> (Updated June 4, 2018, 3:27 p.m.)
> 
> 
> Review request for mesos and Alexander Rojas.
> 
> 
> Bugs: MESOS-8974
> https://issues.apache.org/jira/browse/MESOS-8974
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
> 
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
> 
> The parser only supports parsing of  RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819 
>   3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION 
>   3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67425/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Clement Michaud
> 
>

Re: Review Request 66035: Updated mesos-tidy setup to be based on upstream 6.0 release.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On March 13, 2018, 9:49 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66035/
> ---
> 
> (Updated March 13, 2018, 9:49 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-tidy setup to be based on upstream 6.0 release.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/Dockerfile f046a1410a3012509babb0558998f33b9e241c43 
> 
> 
> Diff: https://reviews.apache.org/r/66035/diff/1/
> 
> 
> Testing
> ---
> 
> * LLVM unit test suite passes
> * new image does not produce false positives
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67425: [WIP] Implement a JWK Set parser converting JWKs into RSA keys.

[Clement Michaud]

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

(Updated juin 4, 2018, 1:27 après-midi)


Review request for mesos.


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


Repository: mesos


Description
---

JWT implementation can handle multiple type of keys for signing and
validating JSON web tokens. IETF defined a JSON representation of those
keys in
https://tools.ietf.org/html/rfc7517. This review implements this RFC.

This commit adds a parser to convert a JSON into a JWK set containing
RSA keys compatible with the implementation of RS256 in the JWT library.

The parser only supports parsing of  RSA keys for now but it can support
multiple types of keys such as elliptic curve keys and symmetric keys
as documented in the JWA RFC.


Diffs
-

  3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819 
  3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION 
  3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION 
  3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67425/diff/1/


Testing
---

make check


Thanks,

Clement Michaud

Re: Review Request 67431: Removed unused libprocess test tools.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67431]

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

- Mesos Reviewbot


On June 4, 2018, 8:36 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67431/
> ---
> 
> (Updated June 4, 2018, 8:36 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused libprocess test tools.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/test-master.cpp 
> 5026af32c6d72d3e031ebf265680ab7bbf937435 
>   3rdparty/libprocess/src/test-slave.cpp 
> 4516bdca66de5889c3163ff7d6890a9806dc4322 
> 
> 
> Diff: https://reviews.apache.org/r/67431/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Used `grep` to confimr that the files are unused.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

[Benjamin Bannier]

> On May 31, 2018, 5:48 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/nested_mesos_containerizer_tests.cpp
> > Line 429 (original), 454 (patched)
> > 
> >
> > I still see this test failing in our internal CI, need to investigate 
> > what is amiss here.

The string comparison here was not portable, fixed now.


- Benjamin


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


On June 1, 2018, 5:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 1, 2018, 5:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67431: Removed unused libprocess test tools.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67431 was successfully built and tested.

Reviews applied: `['67431']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67431

- Mesos Reviewbot Windows


On June 4, 2018, 8:36 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67431/
> ---
> 
> (Updated June 4, 2018, 8:36 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused libprocess test tools.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/test-master.cpp 
> 5026af32c6d72d3e031ebf265680ab7bbf937435 
>   3rdparty/libprocess/src/test-slave.cpp 
> 4516bdca66de5889c3163ff7d6890a9806dc4322 
> 
> 
> Diff: https://reviews.apache.org/r/67431/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Used `grep` to confimr that the files are unused.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67431: Removed unused libprocess test tools.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On June 4, 2018, 8:36 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67431/
> ---
> 
> (Updated June 4, 2018, 8:36 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused libprocess test tools.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/test-master.cpp 
> 5026af32c6d72d3e031ebf265680ab7bbf937435 
>   3rdparty/libprocess/src/test-slave.cpp 
> 4516bdca66de5889c3163ff7d6890a9806dc4322 
> 
> 
> Diff: https://reviews.apache.org/r/67431/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Used `grep` to confimr that the files are unused.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 67431: Removed unused libprocess test tools.

[Benjamin Bannier]

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

Review request for mesos, Jan Schlicht and Till Toenshoff.


Repository: mesos


Description
---

Removed unused libprocess test tools.


Diffs
-

  3rdparty/libprocess/src/test-master.cpp 
5026af32c6d72d3e031ebf265680ab7bbf937435 
  3rdparty/libprocess/src/test-slave.cpp 
4516bdca66de5889c3163ff7d6890a9806dc4322 


Diff: https://reviews.apache.org/r/67431/diff/1/


Testing
---

`make check`

Used `grep` to confimr that the files are unused.


Thanks,

Benjamin Bannier