Re: Review Request 71518: Added the test `GarbageCollectorIntegrationTest.ROOT_OrphanContainer`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71501, 71518]

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

- Mesos Reviewbot


On Sept. 20, 2019, 3:06 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71518/
> ---
> 
> (Updated Sept. 20, 2019, 3:06 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-9966
> https://issues.apache.org/jira/browse/MESOS-9966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the test `GarbageCollectorIntegrationTest.ROOT_OrphanContainer`.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 2ea4bcb668e1fcbeb9c598053e4df4d54d17711d 
> 
> 
> Diff: https://reviews.apache.org/r/71518/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> This test will fail without the previous patch r/71501.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71518: Added the test `GarbageCollectorIntegrationTest.ROOT_OrphanContainer`.

[Qian Zhang]

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

(Updated Sept. 20, 2019, 11:06 a.m.)


Review request for mesos and Gilbert Song.


Changes
---

Made this test linux specific.


Summary (updated)
-

Added the test `GarbageCollectorIntegrationTest.ROOT_OrphanContainer`.


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


Repository: mesos


Description (updated)
---

Added the test `GarbageCollectorIntegrationTest.ROOT_OrphanContainer`.


Diffs (updated)
-

  src/tests/gc_tests.cpp 2ea4bcb668e1fcbeb9c598053e4df4d54d17711d 


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

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


Testing
---

sudo make check

This test will fail without the previous patch r/71501.


Thanks,

Qian Zhang

Re: Review Request 71479: Added a test to ensure resources are recovered during agent removal.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71515, 71516, 71476, 71479]

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

- Mesos Reviewbot


On Sept. 12, 2019, 10:51 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71479/
> ---
> 
> (Updated Sept. 12, 2019, 10:51 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Bugs: MESOS-621
> https://issues.apache.org/jira/browse/MESOS-621
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure resources are recovered during agent removal.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f2e2b259af9920133dd22a5b03774d7035821dc 
> 
> 
> Diff: https://reviews.apache.org/r/71479/diff/1/
> 
> 
> Testing
> ---
> 
> Failed on master.
> Pass after r/71476
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71520: Fixed inefficient `hashmap` access patterns.

[Joseph Wu]

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


Ship it!




Nit: There's a typo in the third-last line of the commit description
s/completely in lie of/completely in lieu of/

The typo is also present in the next review ;)

- Joseph Wu


On Sept. 19, 2019, 6:44 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71520/
> ---
> 
> (Updated Sept. 19, 2019, 6:44 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9948
> https://issues.apache.org/jira/browse/MESOS-9948
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes some inefficient access patterns around `hashmap::get`.
> Since this function returns an `Option` it can be used as a shorthand
> for a `contains` check and subsequent creation of a value (`Option`
> always contains a value). It was never not intended and is inefficient
> for `contains` itself (e.g., via `hashmap::get::isSome`), and for cases
> where only access to parts of the value in the `hashmap` is required
> (e.g., to read a member of an optional value). In both these cases we
> neither want nor need to create a temporary, and should instead either
> just use `contains`, or access the value with `hashmap::at` after a
> `contains` check since otherwise we might spend a lot of time creating
> unnecessary temporary values.
> 
> This patch fixes some easily identifiable case found by manually
> grooming the result of the following clang-query command:
> 
> match cxxMemberCallExpr(
>   on(hasType(cxxRecordDecl(hasName("hashmap",
>   unless(
> hasParent(cxxBindTemporaryExpr(
>   hasParent(materializeTemporaryExpr(
> hasParent(exprWithCleanups(
>   hasParent(varDecl(),
>   callee(cxxMethodDecl(hasName("get"
> 
> This most probably misses a lot of cases. Given how easy it is to
> misuse `hashmap::get` we should consider whether it makes sense to get
> rid of it completely in lie of an inlined form trading some additional
> lookups for temporary avoidance,
> 
> Option x = map.contains(k) ? Some(map.at(k)) : Option::none();
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> 623d44adbe838f995ddbe89ee26f5bcc9c600be5 
> 
> 
> Diff: https://reviews.apache.org/r/71520/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71476: Simplified recover resources when removing frameworks or agents.

[Meng Zhu]

> On Sept. 18, 2019, 10:43 a.m., Andrei Sekretenko wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1489-1494 (original), 1523-1528 (patched)
> > 
> >
> > Previously, this code was executed even in case of recovering resources 
> > of already removed slave.
> > 
> > Now I don't see any framework untracking code in `removeSlave()` or 
> > `untrackAllocatedResources()`. 
> > 
> > Does this mean that now we are potentially leaking frameworks? Or not?

That's a very good point! I refactored (un)trackFrameworkUnderRole in dependecy 
patches. It should be more intuitive now.


- Meng


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


On Sept. 12, 2019, 3:42 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71476/
> ---
> 
> (Updated Sept. 12, 2019, 3:42 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Bugs: MESOS-621
> https://issues.apache.org/jira/browse/MESOS-621
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simplified recover resources when removing frameworks or agents.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5ea37912dadded47eaa2d354889c95533b191c59 
>   src/master/allocator/mesos/hierarchical.cpp 
> 256fdce61ccfb768605cac1579c9a6632cd26fec 
>   src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 
> 
> 
> Diff: https://reviews.apache.org/r/71476/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> New test in https://reviews.apache.org/r/71479/
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71476: Simplified recover resources when removing frameworks or agents.

[Meng Zhu]

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

(Updated Sept. 19, 2019, 5 p.m.)


Review request for mesos, Andrei Sekretenko and Benjamin Mahler.


Changes
---

Rebased on (un)trackFrameworkUnderRole refactor patches.


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


Repository: mesos


Description
---

Simplified recover resources when removing frameworks or agents.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
5ea37912dadded47eaa2d354889c95533b191c59 
  src/master/allocator/mesos/hierarchical.cpp 
256fdce61ccfb768605cac1579c9a6632cd26fec 
  src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 


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

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


Testing
---

make check
New test in https://reviews.apache.org/r/71479/


Thanks,

Meng Zhu

Re: Review Request 71512: Windows: Libprocess: Fixed parallel test execution.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71510, 71511, 71512]

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

- Mesos Reviewbot


On Sept. 19, 2019, 6:30 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71512/
> ---
> 
> (Updated Sept. 19, 2019, 6:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For the test runner script to find the executable now, it needs
> to be given the configuration and the file extension.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> b4ec9907d16c89b45562f4fa33c9f3d2913f6991 
> 
> 
> Diff: https://reviews.apache.org/r/71512/diff/2/
> 
> 
> Testing
> ---
> 
> cmake --build . --target check (Windows)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71496, 71497]

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

- Mesos Reviewbot


On Sept. 19, 2019, 2:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71497/
> ---
> 
> (Updated Sept. 19, 2019, 2:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Bugs: MESOS-9972
> https://issues.apache.org/jira/browse/MESOS-9972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
> `LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.
> 
> The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
> `LIBPROCESS_SSL_VERIFY_SERVER_CERT`.
> 
> The new names better describe the actual effect of both flags, and
> make upgrades easier by allowing operators to only enable verification
> on agents that are new enough to contain the updated hostname
> validation code paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 1a0e3820cc8cd1459625f46a54b194133500f11e 
>   3rdparty/libprocess/src/openssl.hpp 
> 271cc95238d287c06df36478554502e8b7205b09 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 9d5ab679165a709f7c3740020961ec89a7db4f54 
>   docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
>   docs/upgrades.md e630731c332fdd7df788f96644a8084f30b5c621 
> 
> 
> Diff: https://reviews.apache.org/r/71497/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71511: Windows: Stout: Fixed parallel test execution.

[Joseph Wu]

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

(Updated Sept. 19, 2019, 11:30 a.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.


Changes
---

Added comment explaining why the `$<>` expressions are present.


Repository: mesos


Description
---

For the test runner script to find the executable now, it needs
to be given the configuration and the file extension.


Diffs (updated)
-

  3rdparty/stout/tests/CMakeLists.txt e3291c72d1875f5b196dd5ea2ba20e7d53881b87 


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

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


Testing
---

cmake --build . --target check (Windows)


Thanks,

Joseph Wu

Re: Review Request 71512: Windows: Libprocess: Fixed parallel test execution.

[Joseph Wu]

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

(Updated Sept. 19, 2019, 11:30 a.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.


Changes
---

Added comment explaining why the `$<>` expressions are present.


Repository: mesos


Description
---

For the test runner script to find the executable now, it needs
to be given the configuration and the file extension.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/CMakeLists.txt 
b4ec9907d16c89b45562f4fa33c9f3d2913f6991 


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

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


Testing
---

cmake --build . --target check (Windows)


Thanks,

Joseph Wu

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Joseph Wu]

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

(Updated Sept. 19, 2019, 11:30 a.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.


Changes
---

* Added comment blurbs above the main changes.
* Accepted Benjamin's suggested change to mesos-gtest-runner.py.


Repository: mesos


Description
---

Since the default 'check' target now uses the parallel test runner,
there are some changes necessary to use this on Windows.

In the script itself, references to RLimits need to be removed,
since those structures do not exist on Windows.

The TEST_RUNNER variable must include "python" on Windows, since
'.py' files are not automatically run with Python on Windows.

The script also needs the full test executable name (+ '.exe').
We could omit this previously, because you can run executables
while omitting the extension.  But the script checks if the file
exists, and that operation requires the full name plus extension.


Diffs (updated)
-

  cmake/MesosConfigure.cmake 83d41addcd2c14358fba8bab2ac654475626a3e8 
  src/tests/CMakeLists.txt 1e53b396569bf3e2f47199956a630afb6197b992 
  support/mesos-gtest-runner.py 42c0a143477b5ccd411db482a8877e596f520342 


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

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


Testing
---

cmake --build . --target check (Windows)


Thanks,

Joseph Wu

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Joseph Wu]

> On Sept. 19, 2019, 3:25 a.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 55 (patched)
> > 
> >
> > A feature of the current parallel test setup is that users can 
> > overwrite `TEST_DRIVER` at configure time (in autotools even at execution 
> > time). With this change we hardcode the assumption that `TEST_DRIVER` is a 
> > Python executable on Windows.
> > 
> > I am not sure how to address this (e.g., setting the default 
> > `TEST_DRIVER` to `python ...` is not possible as CTest would look for a 
> > non-existing executable name `python ...`). Would linking/copying 
> > `mesos-gtest-runner.py` to a file `mesos-gtest-runner.py.exe` in the build 
> > tree at configure time and adjusting the default for `WIN32` be possible?
> > 
> > In any case not an important regression, feel free to drop.
> 
> Joseph Wu wrote:
> It should still be possible to override the `TEST_DRIVER` here.  The 
> reason for the `TEST_RUNNER` temporary variable is to transform the 
> `TEST_DRIVER` into a list on Windows, but keep it as a string on Posix.
> 
> So on Windows, the variable looks like: 
> `python;../support/mesos-gtest-runner.py`.
> 
> If you were to override the value, (i.e. `cmake .. -DTEST_DRIVER=...`) 
> that value should be kept in the cache, like before.
> 
> Benjamin Bannier wrote:
> Does it prefix the command with `python` on `WIN32` when executing?

I'll add a comment explaining this.

Basically, there are two cases:

---

On Posix:
`TEST_RUNNER` is not set.

So this line:
```
  set(TEST_DRIVER ${TEST_RUNNER} 
"${PROJECT_SOURCE_DIR}/support/mesos-gtest-runner.py" CACHE STRING)
```

Evaluates to:
```
TEST_DRIVER=../support/mesos-gtest-runner.py
```

This is exactly the same as before.

---

On Windows:
```
TEST_RUNNER=python
```

So the same `TEST_DRIVER` expression evaluates to:
```
TEST_DRIVER=python;../support/mesos-gtest-runner.py
```

---

The important detail is when we use `${TEST_DRIVER}` as part of a command.  i.e.
```
add_test(
  NAME MesosTests
  COMMAND
${TEST_DRIVER}
"${CMAKE_BINARY_DIR}/src/mesos-tests")
```

`${TEST_DRIVER}` is a list on Windows, resulting in a proper command like:
```
"python" "../support/mesos-gtest-runner.py" "build/src/mesos-tests"
```

Note that quoting `"${TEST_DRIVER}"` would result in incorrectness, since a 
quoted variable becomes a single argument.


> On Sept. 19, 2019, 3:25 a.m., Benjamin Bannier wrote:
> > src/tests/CMakeLists.txt
> > Lines 390 (patched)
> > 
> >
> > In follow-up patches you used a different logic,
> > 
> > 
> > $<$>:$/>stout-tests$<$:.exe>
> > 
> > Why do they need to be different? Is the `CONFIG` part even required 
> > there?
> 
> Joseph Wu wrote:
> The extra stuff around the test name mostly becomes required due to the 
> `mesos-gtest-runner.py` script.  The script checks if the test binary exists 
> before running it.  And the script can't find the test binary unless we 
> prefix with the build config on Windows; and suffix with the actual file name 
> extension.
> 
> Prior to your test runner change, we didn't need the `CONFIG` part.  And 
> I believe that was cmake extending the binary search path automatically.
> 
> Note: On Windows, if you have an executable like `test.exe`, you can run 
> it while omitting the `.exe`.
> 
> Benjamin Bannier wrote:
> Okay. Why is that not required in the follow-up stout and libprocess 
> patches? In any case, it would go great for maintainability to document that 
> in the actual cmake setup.

The reason mesos-tests don't need the CONFIG is because we do the following in 
`src/CMakeLists.txt`:
```
# Generate all binaries in the same folder.
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/src)
if (WIN32)
  SET(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/src)
  SET(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/src)
endif ()
```

This was for convenience (of finding the binaries) and to produce a similar 
build output directory as the autotools build.


- Joseph


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


On Sept. 18, 2019, 4 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71510/
> ---
> 
> (Updated Sept. 18, 2019, 4 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the default 'check' target now uses the parallel test runner,
> 

Re: Review Request 71520: Fixed inefficient `hashmap` access patterns.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71519, 71520]

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

- Mesos Reviewbot


On Sept. 19, 2019, 6:44 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71520/
> ---
> 
> (Updated Sept. 19, 2019, 6:44 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9948
> https://issues.apache.org/jira/browse/MESOS-9948
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes some inefficient access patterns around `hashmap::get`.
> Since this function returns an `Option` it can be used as a shorthand
> for a `contains` check and subsequent creation of a value (`Option`
> always contains a value). It was never not intended and is inefficient
> for `contains` itself (e.g., via `hashmap::get::isSome`), and for cases
> where only access to parts of the value in the `hashmap` is required
> (e.g., to read a member of an optional value). In both these cases we
> neither want nor need to create a temporary, and should instead either
> just use `contains`, or access the value with `hashmap::at` after a
> `contains` check since otherwise we might spend a lot of time creating
> unnecessary temporary values.
> 
> This patch fixes some easily identifiable case found by manually
> grooming the result of the following clang-query command:
> 
> match cxxMemberCallExpr(
>   on(hasType(cxxRecordDecl(hasName("hashmap",
>   unless(
> hasParent(cxxBindTemporaryExpr(
>   hasParent(materializeTemporaryExpr(
> hasParent(exprWithCleanups(
>   hasParent(varDecl(),
>   callee(cxxMethodDecl(hasName("get"
> 
> This most probably misses a lot of cases. Given how easy it is to
> misuse `hashmap::get` we should consider whether it makes sense to get
> rid of it completely in lie of an inlined form trading some additional
> lookups for temporary avoidance,
> 
> Option x = map.contains(k) ? Some(map.at(k)) : Option::none();
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> 623d44adbe838f995ddbe89ee26f5bcc9c600be5 
> 
> 
> Diff: https://reviews.apache.org/r/71520/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Benjamin Bannier]

> On Sept. 19, 2019, 12:25 p.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 55 (patched)
> > 
> >
> > A feature of the current parallel test setup is that users can 
> > overwrite `TEST_DRIVER` at configure time (in autotools even at execution 
> > time). With this change we hardcode the assumption that `TEST_DRIVER` is a 
> > Python executable on Windows.
> > 
> > I am not sure how to address this (e.g., setting the default 
> > `TEST_DRIVER` to `python ...` is not possible as CTest would look for a 
> > non-existing executable name `python ...`). Would linking/copying 
> > `mesos-gtest-runner.py` to a file `mesos-gtest-runner.py.exe` in the build 
> > tree at configure time and adjusting the default for `WIN32` be possible?
> > 
> > In any case not an important regression, feel free to drop.
> 
> Joseph Wu wrote:
> It should still be possible to override the `TEST_DRIVER` here.  The 
> reason for the `TEST_RUNNER` temporary variable is to transform the 
> `TEST_DRIVER` into a list on Windows, but keep it as a string on Posix.
> 
> So on Windows, the variable looks like: 
> `python;../support/mesos-gtest-runner.py`.
> 
> If you were to override the value, (i.e. `cmake .. -DTEST_DRIVER=...`) 
> that value should be kept in the cache, like before.

Does it prefix the command with `python` on `WIN32` when executing?


> On Sept. 19, 2019, 12:25 p.m., Benjamin Bannier wrote:
> > src/tests/CMakeLists.txt
> > Lines 390 (patched)
> > 
> >
> > In follow-up patches you used a different logic,
> > 
> > 
> > $<$>:$/>stout-tests$<$:.exe>
> > 
> > Why do they need to be different? Is the `CONFIG` part even required 
> > there?
> 
> Joseph Wu wrote:
> The extra stuff around the test name mostly becomes required due to the 
> `mesos-gtest-runner.py` script.  The script checks if the test binary exists 
> before running it.  And the script can't find the test binary unless we 
> prefix with the build config on Windows; and suffix with the actual file name 
> extension.
> 
> Prior to your test runner change, we didn't need the `CONFIG` part.  And 
> I believe that was cmake extending the binary search path automatically.
> 
> Note: On Windows, if you have an executable like `test.exe`, you can run 
> it while omitting the `.exe`.

Okay. Why is that not required in the follow-up stout and libprocess patches? 
In any case, it would go great for maintainability to document that in the 
actual cmake setup.


- Benjamin


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


On Sept. 19, 2019, 1 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71510/
> ---
> 
> (Updated Sept. 19, 2019, 1 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the default 'check' target now uses the parallel test runner,
> there are some changes necessary to use this on Windows.
> 
> In the script itself, references to RLimits need to be removed,
> since those structures do not exist on Windows.
> 
> The TEST_RUNNER variable must include "python" on Windows, since
> '.py' files are not automatically run with Python on Windows.
> 
> The script also needs the full test executable name (+ '.exe').
> We could omit this previously, because you can run executables
> while omitting the extension.  But the script checks if the file
> exists, and that operation requires the full name plus extension.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake 83d41addcd2c14358fba8bab2ac654475626a3e8 
>   src/tests/CMakeLists.txt 1e53b396569bf3e2f47199956a630afb6197b992 
>   support/mesos-gtest-runner.py 42c0a143477b5ccd411db482a8877e596f520342 
> 
> 
> Diff: https://reviews.apache.org/r/71510/diff/1/
> 
> 
> Testing
> ---
> 
> cmake --build . --target check (Windows)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71518: Added the test `GarbageCollectorIntegrationTest.OrphanContainer`.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71501, 71518]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_71518"]

Error:
..
located: storage/default-role):2; mem(allocated: storage/default-role):1024; 
disk(allocated: storage/default-role):1024 (total: cpus:2; mem:1024; disk:1024; 
ports:[31000-32000]; disk(reservations: 
[(DYNAMIC,storage),(DYNAMIC,storage/default-role,test-principal)])[MOUNT(org.apache.mesos.csi.test.local,/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_zHLLLc/2GB-c042af93-66e7-4215-af26-72013ff85317,test)]:2048,
 offered or allocated: {}) on agent 36c92ce8-38aa-4a77-a380-06d69b1beebc-S0 
from framework 36c92ce8-38aa-4a77-a380-06d69b1beebc-
I0919 14:44:25.709256 18652 hierarchical.cpp:1558] Framework 
36c92ce8-38aa-4a77-a380-06d69b1beebc- filtered agent 
36c92ce8-38aa-4a77-a380-06d69b1beebc-S0 for 5secs
I0919 14:44:25.711563 18634 master.cpp:12557] Sending operation '' (uuid: 
6ff76f8a-681e-4043-b580-a52aa2c6d2c7) to agent 
36c92ce8-38aa-4a77-a380-06d69b1beebc-S0 at slave(1248)@172.17.0.3:44477 
(1b384509bb52)
I0919 14:44:25.712173 18650 slave.cpp:4352] Ignoring new checkpointed resources 
and operations identical to the current version
I0919 14:44:25.715298 18639 provider.cpp:498] Received APPLY_OPERATION event
I0919 14:44:25.715380 18639 provider.cpp:1351] Received CREATE operation '' 
(uuid: 6ff76f8a-681e-4043-b580-a52aa2c6d2c7)
I0919 14:44:25.721730 18638 master.cpp:6398] Processing REVIVE call for 
framework 36c92ce8-38aa-4a77-a380-06d69b1beebc- (default) at 
scheduler-d265b96b-d3b0-40b3-9a28-427d737db487@172.17.0.3:44477
I0919 14:44:25.70 18631 hierarchical.cpp:1654] Unsuppressed offers and 
cleared filters for roles { storage/default-role } of framework 
36c92ce8-38aa-4a77-a380-06d69b1beebc-
I0919 14:44:25.723402 18631 hierarchical.cpp:1786] Performed allocation for 1 
agents in 996549ns
I0919 14:44:25.723737 18631 hierarchical.cpp:1786] Performed allocation for 1 
agents in 145181ns
I0919 14:44:25.724021 18637 master.cpp:10389] Sending offers [ 
36c92ce8-38aa-4a77-a380-06d69b1beebc-O4 ] to framework 
36c92ce8-38aa-4a77-a380-06d69b1beebc- (default) at 
scheduler-d265b96b-d3b0-40b3-9a28-427d737db487@172.17.0.3:44477
I0919 14:44:25.724589 18648 sched.cpp:934] Scheduler::resourceOffers took 
76312ns
I0919 14:44:25.732537 18654 http.cpp:1115] HTTP POST for 
/slave(1248)/api/v1/resource_provider from 172.17.0.3:55102
I0919 14:44:25.733646 18650 slave.cpp:8482] Handling resource provider message 
'UPDATE_OPERATION_STATUS: (uuid: 18778154-fd20-4e09-ada8-d19502ac05ca) for 
framework  (latest state: OPERATION_FINISHED, status update state: 
OPERATION_FINISHED)'
I0919 14:44:25.733819 18650 slave.cpp:8935] Updating the state of operation 
with no ID (uuid: 18778154-fd20-4e09-ada8-d19502ac05ca) for an operation API 
call (latest state: OPERATION_FINISHED, status update state: OPERATION_FINISHED)
I0919 14:44:25.733873 18650 slave.cpp:8689] Forwarding status update of 
operation with no ID (operation_uuid: 18778154-fd20-4e09-ada8-d19502ac05ca) for 
an operator API call
I0919 14:44:25.734203 18632 master.cpp:12213] Updating the state of operation 
'' (uuid: 18778154-fd20-4e09-ada8-d19502ac05ca) for an operator API call 
(latest state: OPERATION_PENDING, status update state: OPERATION_FINISHED)
I0919 14:44:25.734715 18651 slave.cpp:4352] Ignoring new checkpointed resources 
and operations identical to the current version
I0919 14:44:25.802605 18648 status_update_manager_process.hpp:152] Received 
operation status update OPERATION_FINISHED (Status UUID: 
80541db7-67e3-4040-89d6-cc2d45370739) for operation UUID 
6ff76f8a-681e-4043-b580-a52aa2c6d2c7 on agent 
36c92ce8-38aa-4a77-a380-06d69b1beebc-S0
I0919 14:44:25.802673 18648 status_update_manager_process.hpp:414] Creating 
operation status update stream 6ff76f8a-681e-4043-b580-a52aa2c6d2c7 
checkpoint=true
I0919 14:44:25.802695 18639 provider.cpp:498] Received 
ACKNOWLEDGE_OPERATION_STATUS event
I0919 14:44:25.802994 18648 status_update_manager_process.hpp:929] 
Checkpointing UPDATE for operation status update OPERATION_FINISHED (Status 
UUID: 80541db7-67e3-4040-89d6-cc2d45370739) for operation UUID 
6ff76f8a-681e-4043-b580-a52aa2c6d2c7 on agent 
36c92ce8-38aa-4a77-a380-06d69b1beebc-S0
I0919 14:44:25.852852 18648 status_update_manager_process.hpp:528] Forwarding 
operation status update OPERATION_FINISHED (Status UUID: 
80541db7-67e3-4040-89d6-cc2d45370739) for operation UUID 
6ff76f8a-681e-4043-b580-a52aa2c6d2c7 on 

Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

[Benno Evers]

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

(Updated Sept. 19, 2019, 2:35 p.m.)


Review request for mesos, Greg Mann and Till Toenshoff.


Changes
---

Revert usage of libprocess DeprecatedName machinery


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


Repository: mesos


Description
---

The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
`LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.

The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
`LIBPROCESS_SSL_VERIFY_SERVER_CERT`.

The new names better describe the actual effect of both flags, and
make upgrades easier by allowing operators to only enable verification
on agents that are new enough to contain the updated hostname
validation code paths.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
1a0e3820cc8cd1459625f46a54b194133500f11e 
  3rdparty/libprocess/src/openssl.hpp 271cc95238d287c06df36478554502e8b7205b09 
  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
9d5ab679165a709f7c3740020961ec89a7db4f54 
  docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
  docs/upgrades.md e630731c332fdd7df788f96644a8084f30b5c621 


Diff: https://reviews.apache.org/r/71497/diff/4/

Changes: https://reviews.apache.org/r/71497/diff/3-4/


Testing
---


Thanks,

Benno Evers

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Joseph Wu]

> On Sept. 19, 2019, 3:25 a.m., Benjamin Bannier wrote:
> > cmake/MesosConfigure.cmake
> > Lines 55 (patched)
> > 
> >
> > A feature of the current parallel test setup is that users can 
> > overwrite `TEST_DRIVER` at configure time (in autotools even at execution 
> > time). With this change we hardcode the assumption that `TEST_DRIVER` is a 
> > Python executable on Windows.
> > 
> > I am not sure how to address this (e.g., setting the default 
> > `TEST_DRIVER` to `python ...` is not possible as CTest would look for a 
> > non-existing executable name `python ...`). Would linking/copying 
> > `mesos-gtest-runner.py` to a file `mesos-gtest-runner.py.exe` in the build 
> > tree at configure time and adjusting the default for `WIN32` be possible?
> > 
> > In any case not an important regression, feel free to drop.

It should still be possible to override the `TEST_DRIVER` here.  The reason for 
the `TEST_RUNNER` temporary variable is to transform the `TEST_DRIVER` into a 
list on Windows, but keep it as a string on Posix.

So on Windows, the variable looks like: 
`python;../support/mesos-gtest-runner.py`.

If you were to override the value, (i.e. `cmake .. -DTEST_DRIVER=...`) that 
value should be kept in the cache, like before.


> On Sept. 19, 2019, 3:25 a.m., Benjamin Bannier wrote:
> > src/tests/CMakeLists.txt
> > Lines 390 (patched)
> > 
> >
> > In follow-up patches you used a different logic,
> > 
> > 
> > $<$>:$/>stout-tests$<$:.exe>
> > 
> > Why do they need to be different? Is the `CONFIG` part even required 
> > there?

The extra stuff around the test name mostly becomes required due to the 
`mesos-gtest-runner.py` script.  The script checks if the test binary exists 
before running it.  And the script can't find the test binary unless we prefix 
with the build config on Windows; and suffix with the actual file name 
extension.

Prior to your test runner change, we didn't need the `CONFIG` part.  And I 
believe that was cmake extending the binary search path automatically.

Note: On Windows, if you have an executable like `test.exe`, you can run it 
while omitting the `.exe`.


- Joseph


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


On Sept. 18, 2019, 4 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71510/
> ---
> 
> (Updated Sept. 18, 2019, 4 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the default 'check' target now uses the parallel test runner,
> there are some changes necessary to use this on Windows.
> 
> In the script itself, references to RLimits need to be removed,
> since those structures do not exist on Windows.
> 
> The TEST_RUNNER variable must include "python" on Windows, since
> '.py' files are not automatically run with Python on Windows.
> 
> The script also needs the full test executable name (+ '.exe').
> We could omit this previously, because you can run executables
> while omitting the extension.  But the script checks if the file
> exists, and that operation requires the full name plus extension.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake 83d41addcd2c14358fba8bab2ac654475626a3e8 
>   src/tests/CMakeLists.txt 1e53b396569bf3e2f47199956a630afb6197b992 
>   support/mesos-gtest-runner.py 42c0a143477b5ccd411db482a8877e596f520342 
> 
> 
> Diff: https://reviews.apache.org/r/71510/diff/1/
> 
> 
> Testing
> ---
> 
> cmake --build . --target check (Windows)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 71519: Fixed inefficient `hashmap` access patterns.

[Benjamin Bannier]

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

Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch fixes some inefficient access patterns around `hashmap::get`.
Since this function returns an `Option` it can be used as a shorthand
for a `contains` check and subsequent creation of a value (`Option`
always contains a value). It was never not intended and is inefficient
for `contains` itself (e.g., via `hashmap::get::isSome`), and for cases
where only access to parts of the value in the `hashmap` is required
(e.g., to read a member of an optional value). In both these cases we
neither want nor need to create a temporary, and should instead either
just use `contains`, or access the value with `hashmap::at` after a
`contains` check since otherwise we might spend a lot of time creating
unnecessary temporary values.

This patch fixes some easily identifiable case found by manually
grooming the result of the following clang-query command:

match cxxMemberCallExpr(
  on(hasType(cxxRecordDecl(hasName("hashmap",
  unless(
hasParent(cxxBindTemporaryExpr(
  hasParent(materializeTemporaryExpr(
hasParent(exprWithCleanups(
  hasParent(varDecl(),
  callee(cxxMethodDecl(hasName("get"

This most probably misses a lot of cases. Given how easy it is to
misuse `hashmap::get` we should consider whether it makes sense to get
rid of it completely in lie of an inlined form trading some additional
lookups for temporary avoidance,

Option x = map.contains(k) ? Some(map.at(k)) : Option::none();


Diffs
-

  src/exec/exec.cpp 67e082e709ef803f49646da5c36147158330f725 
  src/executor/executor.cpp 664a2f1e0723f1afd9220e86e47e86cda67f6b9a 
  src/files/files.cpp f200d5e797084a2a5137ac8f8ede1f6243f12cfb 
  src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 
  src/master/metrics.cpp 20d7231888e263cb3c1759407a2476291e515d4a 
  src/slave/containerizer/fetcher.cpp 8ae789a9f260645e574bbe46a108c3b8cab44cc4 
  src/slave/containerizer/mesos/isolators/posix.hpp 
1ff942cfd1f6829bdc3661b9260dd8e53732d023 
  src/slave/containerizer/mesos/launcher.cpp 
413cc621ad49150a6ddaf689bb75b9dc44741563 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
bf3908d274f8b6f4a57998cbb0a62312b71e3856 
  src/slave/slave.cpp 96890d37ec024fc33d2403b32576776888bcd9f7 
  src/state/log.cpp d3bf2ccceff1934785889ddc9507a754cce445fe 
  src/tests/slave_recovery_tests.cpp d99752ab082f1aca9fb77659378d0bca5a0598eb 
  src/tests/task_status_update_manager_tests.cpp 
0c8b0586a760683ff0ab0f11bf658073087eac12 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 71520: Fixed inefficient `hashmap` access patterns.

[Benjamin Bannier]

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

Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch fixes some inefficient access patterns around `hashmap::get`.
Since this function returns an `Option` it can be used as a shorthand
for a `contains` check and subsequent creation of a value (`Option`
always contains a value). It was never not intended and is inefficient
for `contains` itself (e.g., via `hashmap::get::isSome`), and for cases
where only access to parts of the value in the `hashmap` is required
(e.g., to read a member of an optional value). In both these cases we
neither want nor need to create a temporary, and should instead either
just use `contains`, or access the value with `hashmap::at` after a
`contains` check since otherwise we might spend a lot of time creating
unnecessary temporary values.

This patch fixes some easily identifiable case found by manually
grooming the result of the following clang-query command:

match cxxMemberCallExpr(
  on(hasType(cxxRecordDecl(hasName("hashmap",
  unless(
hasParent(cxxBindTemporaryExpr(
  hasParent(materializeTemporaryExpr(
hasParent(exprWithCleanups(
  hasParent(varDecl(),
  callee(cxxMethodDecl(hasName("get"

This most probably misses a lot of cases. Given how easy it is to
misuse `hashmap::get` we should consider whether it makes sense to get
rid of it completely in lie of an inlined form trading some additional
lookups for temporary avoidance,

Option x = map.contains(k) ? Some(map.at(k)) : Option::none();


Diffs
-

  3rdparty/libprocess/src/metrics/metrics.cpp 
623d44adbe838f995ddbe89ee26f5bcc9c600be5 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71478: Windows: Moved definition out of inline function call.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On Sept. 12, 2019, 7:35 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71478/
> ---
> 
> (Updated Sept. 12, 2019, 7:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MSVC does not deal with #ifdefs from inside function calls.
> So here, the `#if defined(...)` was taken literally and is
> considered a syntax error by MSVC.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
> 
> 
> Diff: https://reviews.apache.org/r/71478/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 71518: Added the test `GarbageCollectorIntegrationTest.OrphanContainer`.

[Qian Zhang]

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

Added the test `GarbageCollectorIntegrationTest.OrphanContainer`.


Diffs
-

  src/tests/gc_tests.cpp 2ea4bcb668e1fcbeb9c598053e4df4d54d17711d 


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


Testing
---

sudo make check

This test will fail without the previous patch r/71501.


Thanks,

Qian Zhang

Re: Review Request 71510: Windows: Fixed parallel test execution.

[Benjamin Bannier]

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




cmake/MesosConfigure.cmake
Lines 55 (patched)


A feature of the current parallel test setup is that users can overwrite 
`TEST_DRIVER` at configure time (in autotools even at execution time). With 
this change we hardcode the assumption that `TEST_DRIVER` is a Python 
executable on Windows.

I am not sure how to address this (e.g., setting the default `TEST_DRIVER` 
to `python ...` is not possible as CTest would look for a non-existing 
executable name `python ...`). Would linking/copying `mesos-gtest-runner.py` to 
a file `mesos-gtest-runner.py.exe` in the build tree at configure time and 
adjusting the default for `WIN32` be possible?

In any case not an important regression, feel free to drop.



src/tests/CMakeLists.txt
Lines 390 (patched)


In follow-up patches you used a different logic,

$<$>:$/>stout-tests$<$:.exe>

Why do they need to be different? Is the `CONFIG` part even required there?



support/mesos-gtest-runner.py
Lines 39 (patched)


Not a big fan of the magic string here and below.

Since this is only used in some best-effort validation I'd suggest we 
instead do down in `validate_setup`:

```{.python}
try:
import resource

/// reset of validation code
except Exception:  /// Also handles `ImportError`.
print(Bcolors.colorize(
   "Could not check compatibility of ulimit settings", /// Drop 
printing of error.
   Bcolors.WARNING),
 file=sys.stderr)
/// Make this a soft error and remove `sys.exit()`.
```


- Benjamin Bannier


On Sept. 19, 2019, 1 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71510/
> ---
> 
> (Updated Sept. 19, 2019, 1 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the default 'check' target now uses the parallel test runner,
> there are some changes necessary to use this on Windows.
> 
> In the script itself, references to RLimits need to be removed,
> since those structures do not exist on Windows.
> 
> The TEST_RUNNER variable must include "python" on Windows, since
> '.py' files are not automatically run with Python on Windows.
> 
> The script also needs the full test executable name (+ '.exe').
> We could omit this previously, because you can run executables
> while omitting the extension.  But the script checks if the file
> exists, and that operation requires the full name plus extension.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake 83d41addcd2c14358fba8bab2ac654475626a3e8 
>   src/tests/CMakeLists.txt 1e53b396569bf3e2f47199956a630afb6197b992 
>   support/mesos-gtest-runner.py 42c0a143477b5ccd411db482a8877e596f520342 
> 
> 
> Diff: https://reviews.apache.org/r/71510/diff/1/
> 
> 
> Testing
> ---
> 
> cmake --build . --target check (Windows)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71441: Fixed URI stringification.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71441]

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

- Mesos Reviewbot


On Sept. 19, 2019, 5:11 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71441/
> ---
> 
> (Updated Sept. 19, 2019, 5:11 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-9963
> https://issues.apache.org/jira/browse/MESOS-9963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the caller is not careful to ensure that the path component begins
> with '/', stringification of URI objects will concatenate the host and
> path, resulting in an malformed URI. The fix is to ensure that these
> fields are always separated by '/'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_spec_tests.cpp 
> 4be0716895132e0406299f7d77f8ceb2d95dbe8a 
>   src/tests/uri_tests.cpp 1d33ab110bed7c0b7ecfa5d608965da5a1729562 
>   src/uri/utils.cpp 3940dc041c1783eec9e7c950402fd7c42e620d8e 
> 
> 
> Diff: https://reviews.apache.org/r/71441/diff/3/
> 
> 
> Testing
> ---
> 
> make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>