Re: Review Request 66004: Updated task reconciliation path to avoid copying in v1.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66004 was successfully built and tested.

Reviews applied: `['66004']`

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

- Mesos Reviewbot Windows


On March 10, 2018, 12:51 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66004/
> ---
> 
> (Updated March 10, 2018, 12:51 a.m.)
> 
> 
> Review request for mesos, Meng Zhu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enforces the pattern of v0 messages converting to a v1
> message and passing through the v1 logic.
> 
> From a performance perspective, the v1 path no longer copies into
> a `vector` which should offer improved performance.
> In the v0 path, we now move into a `Reconcile` message rather than
> moving into a `vector`. Therefore, this improves v1
> performance while keeping v0 performance roughly the same in terms
> of copying.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp cf03d8bf7df583c849a3b2a326d3772820181b0f 
>   src/master/master.hpp 8bf2c763dafdb7df55c46a56f2ff66f2a951d947 
>   src/master/master.cpp f0f6e5b91b4eae99dec52725f5efc203695c41e8 
> 
> 
> Diff: https://reviews.apache.org/r/66004/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66016: Refactored resource allocation logic.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66016']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66016/logs/mesos-tests-stdout.log):

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (130 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (1203 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (40 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (44 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (86 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (2547 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (2571 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (2452 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (2477 ms total)

[--] Global test environment tear-down
[==] 916 tests from 91 test cases ran. (472525 ms total)
[  PASSED  ] 915 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 210 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66016/logs/mesos-tests-stderr.log):

```
I0310 01:49:30.325629 10884 slave.cpp:3878] Shutting down framework 
fd8c9e29-7d14-4533-bb86-fac8e2084c51-
I0310 01:49:30.325629  4588 master.cpp:10214] Updating the state of task 
54c49790-38d0-4357-8934-e1f431b5f286 of framework 
fd8c9e29-7d14-4533-bb86-fac8e2084c51- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0310 01:49:30.325629 10884 slave.cpp:6571] Shutting down executor 
'54c49790-38d0-4357-8934-e1f431b5f286' of framework fd8c9e29-7d14-4533-bb8I0310 
01:49:29.616658  8928 exec.cpp:162] Version: 1.6.0
I0310 01:49:29.646654  6484 exec.cpp:236] Executor registered on agent 
fd8c9e29-7d14-4533-bb86-fac8e2084c51-S0
I0310 01:49:29.651634  8372 executor.cpp:176] Received SUBSCRIBED event
I0310 01:49:29.656656  8372 executor.cpp:180] Subscribed executor on 
build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0310 01:49:29.657655  8372 executor.cpp:176] Received LAUNCH event
I0310 01:49:29.662653  8372 executor.cpp:648] Starting task 
54c49790-38d0-4357-8934-e1f431b5f286
I0310 01:49:29.746654  8372 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0310 01:49:30.289628  8372 executor.cpp:661] Forked command at 7692
I0310 01:49:30.329670  1056 exec.cpp:445] Executor asked to shutdown
I0310 01:49:30.330628  8372 executor.cpp:176] Received SHUTDOWN event
I0310 01:49:30.330628  8372 executor.cpp:758] Shutting down
I0310 01:49:30.330628  8372 executor.cpp:868] Sending SIGTERM to process tree 
at pid 76-fac8e2084c51- at executor(1)@10.3.1.11:50229
I0310 01:49:30.327628 10884 slave.cpp:924] Agent terminating
W0310 01:49:30.328877 10884 slave.cpp:3874] Ignoring shutdown framework 
fd8c9e29-7d14-4533-bb86-fac8e2084c51- because it is terminating
I0310 01:49:30.330628  4588 master.cpp:10313] Removing task 
54c49790-38d0-4357-8934-e1f431b5f286 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework fd8c9e29-7d14-4533-bb86-fac8e2084c51- on 
agent fd8c9e29-7d14-4533-bb86-fac8e2084c51-S0 at slave(398)@10.3.1.11:50208 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0310 01:49:30.331629  4048 containerizer.cpp:2338] Destroying container 
ff5c8033-55fa-43b1-81e4-58cc54c75678 in RUNNING state
I0310 01:49:30.331629  4048 containerizer.cpp:2952] Transitioning the state of 
container ff5c8033-55fa-43b1-81e4-58cc54c75678 from RUNNING to DESTROYING
I0310 01:49:30.333627  4048 launcher.cpp:156] Asked to destroy container 
ff5c8033-55fa-43b1-81e4-58cc54c75678
I0310 01:49:30.333627  4588 master.cpp:1288] Agent 
fd8c9e29-7d14-4533-bb86-fac8e2084c51-S0 at slave(398)@10.3.1.11:50208 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0310 01:49:30.333627 11240 hierarchical.cpp:344] Removed 

Re: Review Request 65851: Disallow limit in /quota requests and SET_QUOTA calls.

[Benjamin Mahler]

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

(Updated March 10, 2018, 1:46 a.m.)


Review request for mesos, Michael Park and Meng Zhu.


Changes
---

Added a TODO per meng's suggestion.


Repository: mesos


Description
---

This ensures that after the protobuf changes to introduce limit
are committed, users cannot set limit in the existing API.


Diffs (updated)
-

  src/master/quota.cpp 58bab6a678bac9e41a7994ba0b7cc1ed069a8a18 


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

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


Testing
---

Test added in the subsequent patch.


Thanks,

Benjamin Mahler

Re: Review Request 65334: Added quota limit to the master API protos.

[Benjamin Mahler]

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

(Updated March 10, 2018, 1:46 a.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Meng Zhu.


Changes
---

Updated to reflect that limits will not undergo an attainability check.


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


Repository: mesos


Description
---

This introduces a `limit` in `QuotaInfo` and `QuotaRequest`.

A new `UPDATE_QUOTA` master call is added as a replacement of
`SET_QUOTA` and `REMOVE_QUOTA`, which provides the new semantics
of unset limit meaning "no limit" and unset guarantee meaning
"no guarantee".

The new APIs are marked as experimental for now.


Diffs (updated)
-

  include/mesos/master/master.proto 6622e1b82861f380346f505eca326f7174dd8bd6 
  include/mesos/quota/quota.proto f2ed6f555e1e7b0290fefa3e301c9bdbf550bd11 
  include/mesos/v1/master/master.proto 6034bd5af8ad764f625f9fe80be08b48707bbadb 
  include/mesos/v1/quota/quota.proto 5dd772fee46f740b062827ed7c9704c26187cb12 
  src/master/http.cpp cf03d8bf7df583c849a3b2a326d3772820181b0f 
  src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 


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

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 66004: Updated task reconciliation path to avoid copying in v1.

[Benjamin Mahler]

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

Review request for mesos, Meng Zhu and Jiang Yan Xu.


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


Repository: mesos


Description
---

This enforces the pattern of v0 messages converting to a v1
message and passing through the v1 logic.

>From a performance perspective, the v1 path no longer copies into
a `vector` which should offer improved performance.
In the v0 path, we now move into a `Reconcile` message rather than
moving into a `vector`. Therefore, this improves v1
performance while keeping v0 performance roughly the same in terms
of copying.


Diffs
-

  src/master/http.cpp cf03d8bf7df583c849a3b2a326d3772820181b0f 
  src/master/master.hpp 8bf2c763dafdb7df55c46a56f2ff66f2a951d947 
  src/master/master.cpp f0f6e5b91b4eae99dec52725f5efc203695c41e8 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 65666: Added a unit test for SLRP operation state metrics.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65666 was successfully built and tested.

Reviews applied: `['65997', '66015', '65995', '65974', '65594', '65975', 
'65976', '65640', '65665', '65666']`

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

- Mesos Reviewbot Windows


On March 9, 2018, 3:29 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65666/
> ---
> 
> (Updated March 9, 2018, 3:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_OperationStateMetrics` test that issues a
> `CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
> fail due to an out-of-band deletion of the actual volume, and the second
> one will fail due to modifying the resource version.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 264d42b57fe1a8ea04c1de0a10112878c7b45d39 
> 
> 
> Diff: https://reviews.apache.org/r/65666/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 66016: Refactored resource allocation logic.

[Meng Zhu]

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

Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael Park, 
and Till Toenshoff.


Repository: mesos


Description
---

The current allocation logic maintains several states
such as allocated-reservation, unsatisfied-quota and etc.
This patch refactors the logic so that we only need to
maintain a per-quota-role map to track role consumed quota.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
5d30d1d4d4bbb5431745f61b5318b39c5c3a7117 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 65936: Changed flags to CreateFile to support Windows symlink path resolution.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65936]

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 March 8, 2018, 11:21 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65936/
> ---
> 
> (Updated March 8, 2018, 11:21 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-8646
> https://issues.apache.org/jira/browse/MESOS-8646
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed flags to CreateFile to support Windows symlink path resolution.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp 
> 858b3c7c27e95b9cf46a7457c1aaa05a0e19188d 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> b84f1ae17246d94947538efeaf504a2cd247f20e 
> 
> 
> Diff: https://reviews.apache.org/r/65936/diff/3/
> 
> 
> Testing
> ---
> 
> I encountered a permissions error stating "The process cannot access the file 
> because it is being used by another process." when attempting to download 
> stderr/stdout from a task running on a Windows agent, or when attempting to 
> download the mesos agent log on a Windows agent. 
> 
> After some very helpful information from Andy and Akash, I made this change 
> and confirmed it fixed this issue with the task/agent logs.
> 
> I also ran the automated tests on Windows and all tests pass.
> 
> I did not test this on Linux, but because this file is compiled only on 
> Windows, it shouldn't be necessary.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 66014: Windows: Made SASL use default CRT linking.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66014 was successfully built and tested.

Reviews applied: `['66007', '66008', '66009', '66010', '66011', '66012', 
'66013', '66014']`

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

- Mesos Reviewbot Windows


On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66014/
> ---
> 
> (Updated March 9, 2018, 10:39 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8659
> https://issues.apache.org/jira/browse/MESOS-8659
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Made SASL use default CRT linking.
> 
> 
> Diffs
> -
> 
>   3rdparty/cyrus-sasl-2.1.27rc3.patch 
> e363b9e92452a3558f5341faf1653514d85364d4 
> 
> 
> Diff: https://reviews.apache.org/r/66014/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65987: Allow nested containers in pods to have separate namespaces(Ref: MESOS-8534).

[Jie Yu]

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



Looking good! Is it possible to add some test?


src/slave/containerizer/mesos/isolators/network/cni/cni.hpp
Lines 114 (patched)


nits: `joinsParentsNetwork`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 389 (original), 389-397 (patched)


Instead of relying on an additional checkpoint file, I think we can infer 
if a nested container joins its parent container's network or not by first 
listing all entries in `rootDir`, and then recover those known containers, and 
then cleanup unknown orphans. Something like the following:

```
// Build ContainerID -> ContainerState mapping
// List rootDir
// For each entry, find the corresponding ContainerState state (state is 
optional)
// Call _recover(containerId, state), if containerId is nested, set 
joinsParentsNetwork to false.
// Cleanup unknown orphans (not in container state mapping or orphans map)
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 576-579 (original), 585-588 (patched)


These should be `const bool`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 577-578 (original), 586-587 (patched)


style nits. We typically do the following

```
bool isDebugContainer =
  containerConfig.container_class() == ContainerClass::DEBUG;
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 589-590 (patched)


style nits: we typically align like this:
```
bool joinParentsNetwork =
  !containerConfig.has_container_info() ||
  containerConfig.container_info().network_infos().empty();
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 613 (patched)


I'd add a comment here:
```
// This is either a top level container, or a nested container
// joining separate network than its parent.
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 581 (original), 613-614 (patched)


It's possible that `containerConfig.has_container_inf() == false` here (top 
level container joining host network without container image).

We typically avoid depending on the default value of `ContainerInfo`. So 
I'd suggest we do:

```
if (containerCOnfig.has_container_info()) {
  const ContainerInfo& containerInfo = containerConfig.container_info();
  
  if (containerInfo.type() != ContainerInfo::MESOS) {
return Failure("Can only prepare CNI networks for a MESOS container");
  }
  ...
}
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 683 (original), 687 (patched)


Not yours, let's reduce nesting by remove this `else` here given that we 
already shortcircuit above `return None()`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 696-698 (original), 700-702 (patched)


Debug container is always nested. So I'd adjust the logic here:
```
if (isDebugContainer) {
  CHECK(isNestedContainer);
  
  // Nested DEBUG containers never need a new MOUNT namespace, so
  // we don't maintain information about them in the `infos` map.
}
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 770 (patched)


hum, this looks problematic. We don't create `Info` struct for debug 
containers.

Also, should we also update that field for the case where `if 
(containerNetworks.empty()) {` above (just to be consistent)?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1269-1285 (patched)


I don't think we need to checkpoint this additional information. You should 
be able to infer if a nested container has its own network namespace or not by 
checking if there's a container directory under `cni::paths::ROOT_DIR`. See 
src/slave/containerizer/mesos/isolators/network/cni/paths.hpp



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 1441 (original)


Why this change?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1488 (patched)


What about top level containers? I think 

Re: Review Request 65666: Added a unit test for SLRP operation state metrics.

[Chun-Hung Hsiao]

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

(Updated March 9, 2018, 11:29 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch adds the `ROOT_OperationStateMetrics` test that issues a
`CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
fail due to an out-of-band deletion of the actual volume, and the second
one will fail due to modifying the resource version.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
264d42b57fe1a8ea04c1de0a10112878c7b45d39 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 66014: Windows: Made SASL use default CRT linking.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66014/
> ---
> 
> (Updated March 9, 2018, 10:39 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Made SASL use default CRT linking.
> 
> 
> Diffs
> -
> 
>   3rdparty/cyrus-sasl-2.1.27rc3.patch 
> e363b9e92452a3558f5341faf1653514d85364d4 
> 
> 
> Diff: https://reviews.apache.org/r/66014/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65665: Added operation state metrics in SLRP.

[Chun-Hung Hsiao]

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

(Updated March 9, 2018, 10:55 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch adds `operations_pending`, `operations_finished`,
`operations_failed`, `operations_error` (currently unused), and
`operations_dropped` metrics to count the occurances of these operation
states.

Additionally, An error log in `_applyOperation()` is removed because the
error is already logged at the call site.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
bb19ed4b6b1b8f5f6b327461a737517497c8c38e 


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

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


Testing
---

sudo make check
A unit test is added in the next patch in chain.


Thanks,

Chun-Hung Hsiao

Review Request 66015: Built CSI spec proto files with cmake.

[Chun-Hung Hsiao]

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

Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


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


Repository: mesos


Description
---

Built CSI spec proto files with cmake.


Diffs
-

  3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 
  3rdparty/cmake/Versions.cmake 94b0d8048412e00e2480f45e7ce4e6494da4bd5d 
  src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 


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


Testing
---

`make check` with cmake.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for cmake.

[Chun-Hung Hsiao]

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

(Updated March 9, 2018, 10:44 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and 
Joseph Wu.


Changes
---

Added a `GRPC` option and made this reade for review.


Summary (updated)
-

Added `PROTOC_SPEC_GENERATE` helper for cmake.


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


Repository: mesos


Description (updated)
---

PROTOC_SPEC_GENERATE is a convenience function that will:
  (1) Compile .proto files found in the third-party specification
  library under its include directory and place the generated files
  in the build folder, under the `include/` directory, or with the
  option `INTERNAL` the `src/` directory.
  (2) Append to list variables `PUBLIC_PROTO_PATH` or
  `INTERNAL_PROTO_PATH` the fully qualified path to the library's
  include directory depending on options passed in.
  (3) Append to list variables `PUBLIC_PROTOBUF_INCLUDE_DIR` or
  `INTERNAL_PROTOBUF_INCLUDE_DIR` (depending on options passed in)
  the fully qualified path to the directory where the files are
  generated.
  (4) Append to list variables `PUBLIC_PROTOBUF_SRC` or
  `INTERNAL_PROTOBUF_SRC` (depending on options passed in) the fully
  qualified path to the generated .cc file.
Where the exports in (2)(3)(4) are *side effects* and modifies the
variables in the parent scope.


Diffs (updated)
-

  src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
  src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505 


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

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


Testing (updated)
---

`make check` with cmake.


Thanks,

Chun-Hung Hsiao

Review Request 66014: Windows: Made SASL use default CRT linking.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


Repository: mesos


Description
---

Windows: Made SASL use default CRT linking.


Diffs
-

  3rdparty/cyrus-sasl-2.1.27rc3.patch e363b9e92452a3558f5341faf1653514d85364d4 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C` and `CXX` versions.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


Repository: mesos


Description
---

By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
libevent, and zlib, we avoid CMake warning us about set but unused
variables (since they do not use the `CXX` variables).


Diffs
-

  3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 66013: Windows: Made ZooKeeper use default CRT linking.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


Repository: mesos


Description
---

Also deleted extraneous patch files.


Diffs
-

  3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 
  3rdparty/zookeeper-06d3f3f.patch 7f66875c7ef8bea3cc077d064b90e056c706487f 
  3rdparty/zookeeper-3.4.8.patch c532a696f8bf3373b7341ddf32dcf9c528ee4412 
  3rdparty/zookeeper-3.5.2-alpha.patch 6d5db536437b22a2903c1fa3348613e12fd35df0 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 66011: Windows: Set 3rdparty libraries to link to CRT dynamically.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


Repository: mesos


Description
---

This also fixes a bug where the `libevent` build on Windows would add
`''` to the compiler flags, which is an unrecognized flag, and sets
the names of our `CMAKE_FORWARD_ARGS` variables consistently (some
were missing the `FORWARD` part).


Diffs
-

  3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 66010: Windows: Switched to default CRT linkage.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


Repository: mesos


Description
---

We previously attempted to manually override the CRT to be static
everywhere. Not only did this emit warnings, it was also error-prone
and unnecessary. We can, and should, just use the defaults, which is
`/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
to the CRT dynamically results in smaller libraries and executables,
reduces linking time, and avoids bugs when sharing allocated memory
across modules.


Diffs
-

  cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 66009: CMake: Added `-Wno-unused-local-typedefs` to Boost interface.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


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


Repository: mesos


Description
---

As found in the Autotools builds, GCC and Clang will emit an used
local typedef warning for headers including Boost. Since it is
3rdparty code, we disable this warning in its interface.


Diffs
-

  3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 66008: CMake: Enabled compiler warnings.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


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


Repository: mesos


Description
---

We had previously been using the default sets of warnings, but now we
use the same warnings as on Autotools. This also means that we can
safely turn on the treat-warnings-as-errors (at least for the Mesos
code). However, doing so requires that we disable two common
possible-loss-of-data warnings on Windows that are not part of the
GNU/Clang default warnings.

This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with
the canonical command `add_compile_options` (though setting these on a
per-target basis would be even better, it's a separate issue).


Diffs
-

  cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
  src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 66007: CMake: Set C++11 as standard automatically.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
Kordich, Joseph Wu, and Michael Park.


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


Repository: mesos


Description
---

Instead of setting the compiler option manually, we use the
`CMAKE_CXX_STANDARD` variable to set the default for all targets. This
automatically appends the correct flag for each compiler.


Diffs
-

  cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65311: Added the ObjectApprovers to which unifies authorization logic.

[Greg Mann]

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




src/common/http.hpp
Lines 198 (patched)


See the comment I left on https://reviews.apache.org/r/65313/ - I wonder if 
`action` really needs to be a template parameter here?


- Greg Mann


On March 8, 2018, 12:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65311/
> ---
> 
> (Updated March 8, 2018, 12:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the class `ObjectApprovers` which unifies the different
> mechanisms used in Mesos in order to perform authorization.
> 
> This new class will supersede the `Acceptor` interfaces and their
> logic.
> 
> Follow up patches will make use of this interface and remove
> deprecated code.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
>   src/common/http.cpp 8d3a4ad63bd74e0313082bad3d89c21fbf54f256 
> 
> 
> Diff: https://reviews.apache.org/r/65311/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 65313: Refactored authorization logic in the agent.

[Greg Mann]

> On March 9, 2018, 5:30 a.m., Greg Mann wrote:
> > src/slave/http.cpp
> > Lines 2641-2665 (original), 2431-2469 (patched)
> > 
> >
> > Why did you opt for a new templated method instead of the pre-existing 
> > lambda, here and elsewhere? Is it necessary because we pass the action as a 
> > template parameter?
> 
> Alexander Rojas wrote:
> It has to do more with the `Approvers::approved()` being parametrize, 
> which means that the type needs to be resolved on compilation time, so this 
> is a neat trick for that.

IMO it's unfortunate that we need to do this, since it makes this code harder 
to read.

Actually, looking again at `ObjectApprovers`, I wonder if the action really 
needs to be a template parameter. We could also do something like:
```
class ObjectApprovers {
  template 
  bool approved(const authorization::Action& action, const Args&... args);

...
```

Then a callsite would look like
```
if (!approvers->approved(SOME_ACTION, someInfo)) {
```
which seems OK to me. WDYT?


- Greg


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


On March 9, 2018, 1:06 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> ---
> 
> (Updated March 9, 2018, 1:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b 
>   src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b 
>   src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219 
> 
> 
> Diff: https://reviews.apache.org/r/65313/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 65311: Added the ObjectApprovers to which unifies authorization logic.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 8, 2018, 12:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65311/
> ---
> 
> (Updated March 8, 2018, 12:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the class `ObjectApprovers` which unifies the different
> mechanisms used in Mesos in order to perform authorization.
> 
> This new class will supersede the `Acceptor` interfaces and their
> logic.
> 
> Follow up patches will make use of this interface and remove
> deprecated code.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
>   src/common/http.cpp 8d3a4ad63bd74e0313082bad3d89c21fbf54f256 
> 
> 
> Diff: https://reviews.apache.org/r/65311/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66000, 66001]

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 March 9, 2018, 5:04 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66001/
> ---
> 
> (Updated March 9, 2018, 5:04 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New Flags for disk/xfs isolator
> - xfs_disk_hard_limit_offset_pct - creates a gap between
> - xfs_disk_hard_limit_offset - overrides _pct creates an offset between
>   soft and hard limits of a container.
> - xfs_kill_after_grace_period - will cause a container to be killed if
>   the soft limit has been breached after the `xfs_quota` configured
>   grace period has ended.
> - xfs_kill_check_interval - Period of delay between which the isolator
>   will check for violations of the soft limit.
> 
> Functionality
> - Add head room to the hard limit as a percentage or specified amount
>   for each container. This is specified at a flag level and not
>   customizable on a per container basis.
> - Provide the ability for an application to be killed after the
>   configured grace period for projects is violated.
> - Add an interval between which the watcher will check for violations.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 07e68a777aefba4dd35066f2eb207bba7f199d83 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 8d9f8f846866f9de377c59cb7fb311041283ba70 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> e034133629a9c1cf58b776f8da2a93421332cee0 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> 2708524add1ff693b616d4fb241c4a0a3070520b 
>   src/slave/flags.hpp 0c67bf214ceb93ae7ff088bec2648fa26ddac59e 
>   src/slave/flags.cpp 943aaaf58b5f36555f0902019b8c5c6522ab7afc 
> 
> 
> Diff: https://reviews.apache.org/r/66001/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 65971: Avoid a copy of the scheduler->executor message in the master.

[Benjamin Mahler]

> On March 9, 2018, 8:01 p.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Lines 5902-5905 (original), 5902-5905 (patched)
> > 
> >
> > Also move this?

This was moved in https://reviews.apache.org/r/65972/


> On March 9, 2018, 8:01 p.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Line 5907 (original), 5907 (patched)
> > 
> >
> > I found this call chain really confusing.
> > This function handles `FrameworkToExecutorMessage` by converting it 
> > into `scheduler::Call::Message` and pass it to `message()`. In `message()`, 
> > we just convert it back to `FrameworkToExecutorMessage` and send it. I 
> > wonder why don't we just send `FrameworkToExecutorMessage` straight away?
> > 
> > This seems to adhere to the pattern of converting internal message 
> > (used by v0) into Call and then process the call and generate other 
> > internal messages as needed. The confusing part here is that the message we 
> > receive happens to be the same to the message we send.
> > 
> > Do you think it is better to just forward it here instead of going 
> > through the unnecessary conversion?
> > Or add some comment regarding the detour?

The pattern for v0 messages is to convert to v1 message and go through the v1 
path, so we should stick to that for consistency. My hope is that with moves as 
opposed to copies the overhead of this transformation becomes negligable. I'm 
not sure if adding a comment here to describe the pattern is a good idea 
because that comment would need to be in every v0 handler, my hope is that 
someone reading the code would soon afterwards discover the v0 pattern (much 
like you did in your comment). We could consider a comment at the `install`s 
within `Master::initialize()` to describe the pattern but would you have read 
that code?


- Benjamin


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


On March 8, 2018, 4:30 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65971/
> ---
> 
> (Updated March 8, 2018, 4:30 a.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This avoids a copy in both the v1 http and v0 message code paths.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65971/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66002: Fixed the HTTP API path variables on Windows.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66002']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66002/logs/mesos-tests-stdout.log):

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (128 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (1223 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (43 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (46 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (92 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (2495 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (2520 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (2478 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (2502 ms total)

[--] Global test environment tear-down
[==] 918 tests from 91 test cases ran. (474724 ms total)
[  PASSED  ] 917 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 208 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66002/logs/mesos-tests-stderr.log):

```
I0309 20:33:19.885869  2232 slave.cpp:3878] Shutting down framework 
7d4e00f9-21e5-42e2-8624-0637b93a0e3b-
I0309 20:33:19.885869  7700 master.cpp:10210] Updating the state of task 
ca531887-b68c-4bce-96ac-514ec6d35811 of framework 
7d4e00f9-21e5-42e2-8624-0637b93a0e3b- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0309 20:33:19.885869  2232 slave.cpp:6571] Shutting down executor 
'ca531887-b68c-4bce-96ac-514ec6d35811' of framework 
7d4e00f9-21e5-42e2-8624-0637b93a0e3b- at executor(1)@10.3.1.11:61477
I0309 20:33:19.70  2232 slave.cpp:924] Agent terminating
W0309 20:33:19.70  2232 slave.cpp:3874] Ignoring shutdown framework 
7d4e00f9-21e5-42e2-8624-0637b93a0e3b- because it is terminating
I0309 20:33:19.889871  7700 master.cpp:10309] Removing task 
ca531887-b68c-4bce-96ac-514ec6d35811 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 7d4e00f9-21e5-42e2-8624-0637b93a0e3b- on 
agent 7d4e00f9-21e5-42e2-8624-0637b93a0e3b-S0 at slave(398)@10.3.1.11:61455 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.I0309 
20:33:19.178894  3528 exec.cpp:162] Version: 1.6.0
I0309 20:33:19.208870  9684 exec.cpp:236] Executor registered on agent 
7d4e00f9-21e5-42e2-8624-0637b93a0e3b-S0
I0309 20:33:19.212896  9144 executor.cpp:176] Received SUBSCRIBED event
I0309 20:33:19.218895  9144 executor.cpp:180] Subscribed executor on 
build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0309 20:33:19.218895  9144 executor.cpp:176] Received LAUNCH event
I0309 20:33:19.223894  9144 executor.cpp:648] Starting task 
ca531887-b68c-4bce-96ac-514ec6d35811
I0309 20:33:19.307874  9144 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0309 20:33:19.848868  9144 executor.cpp:661] Forked command at 3252
I0309 20:33:19.889871  5412 exec.cpp:445] Executor asked to shutdown
I0309 20:33:19.890872  9144 executor.cpp:176] Received SHUTDOWN event
I0309 20:33:19.890872  9144 executor.cpp:758] Shutting down
I0309 20:33:19.890872  9144 executor.cpp:868] Sending SIGTERM to process tree 
at pid 3net)
I0309 20:33:19.893869 10732 containerizer.cpp:2338] Destroying container 
017d2658-dafb-43ee-8e1b-0439b6f41b3f in RUNNING state
I0309 20:33:19.893869 10732 containerizer.cpp:2952] Transitioning the state of 
container 017d2658-dafb-43ee-8e1b-0439b6f41b3f from RUNNING to DESTROYING
I0309 20:33:19.894868 10732 launcher.cpp:156] Asked to destroy container 
017d2658-dafb-43ee-8e1b-0439b6f41b3f
I0309 20:33:19.894868  7700 master.cpp:1306] Agent 
7d4e00f9-21e5-42e2-8624-0637b93a0e3b-S0 at slave(398)@10.3.1.11:61455 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0309 20:33:19.894868  7700 master.cpp:3276] Disconnecting 

Re: Review Request 65968: Updated the master's decline handler to take an rvalue reference.

[Benjamin Mahler]

> On March 8, 2018, 8:54 p.m., Meng Zhu wrote:
> > Adding a TODO to make other handlers also take rvalue?

Rather than adding TODOs on each handler, I tracked the work under MESOS-8628.


- Benjamin


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


On March 8, 2018, 3:52 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65968/
> ---
> 
> (Updated March 8, 2018, 3:52 a.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master's handlers should all take rvalue references for
> consistency.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65968/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65967: Eliminated some copying in the master's Accept call handler path.

[Benjamin Mahler]

> On March 8, 2018, 9:16 p.m., Jiang Yan Xu wrote:
> > Sweet! Would it make sense to group JIRAs into a "copy elimination" epic? 
> > Completing a sweep is a large scale effort so an Epic with subtasks helps 
> > tracking remaining work?

I tried to capture the master's message handler copy-elimination work in 
MESOS-8628. There's probably more copy elimination possible but I would have to 
study the code further.


- Benjamin


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


On March 8, 2018, 3:52 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65967/
> ---
> 
> (Updated March 8, 2018, 3:52 a.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This eliminates two copies of the `Accept` message.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65967/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66002: Fixed the HTTP API path variables on Windows.

[John Kordich via Review Board]

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

(Updated March 9, 2018, 8:17 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, and 
Joseph Wu.


Changes
---

Expanded commit message


Summary (updated)
-

Fixed the HTTP API path variables on Windows.


Repository: mesos


Description (updated)
---

Many mesos frameworks assume that path separators are forward slashes,
like dcos-ui and marathon-ui. dcos-cli also assumes this. Previous to
this patch, if a forward slash was given in the path variable to an HTTP
API function call to the mesos agent on a Windows system, the Windows
API would fail at recognizing the path, because the Windows API accepts
only backslashes as path separators. To remedy this issue, we now
convert all forward slashes passed as a path to the HTTP API to an agent
to back slashes on Windows agents by using the path::from_uri function.


Diffs (updated)
-

  src/files/files.cpp 5f92d2af722c3201200fa1d6a75dc0b87fdc6078 
  src/tests/files_tests.cpp c703cae03345112715aeab83cb0a74abe3e12469 


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

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


Testing
---

Tested on both Windows and Linux, all tests pass, including the two newly 
enabled tests on Windows.


Thanks,

John Kordich

Re: Review Request 65964: Updated the master's status update handler to avoid unnecessary copying.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On March 7, 2018, 7:52 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65964/
> ---
> 
> (Updated March 7, 2018, 7:52 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we took a copy in order to mutate the UUID within the
> update if communicating with an agent prior to 0.26. We now no longer
> need to copy the update.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65964/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65966: Updated the master's receive handler to take an rvalue reference.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On March 7, 2018, 7:52 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65966/
> ---
> 
> (Updated March 7, 2018, 7:52 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to perform copy elimination in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65966/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65965: Avoid copying of launch task messages in the master.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On March 7, 2018, 7:52 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65965/
> ---
> 
> (Updated March 7, 2018, 7:52 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, the launch task message was copied into an
> Accept or Decline call. This updates the handler to perform moves
> instead.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65965/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65969: Avoid a copy of the status update acknowledgements in the master.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On March 7, 2018, 8:24 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65969/
> ---
> 
> (Updated March 7, 2018, 8:24 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the handler to take an rvalue reference and move the
> data into the `scheduler::Call::Acknowledge` message.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65969/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65970: Avoid copying acknowledgement message data in the master.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On March 7, 2018, 8:28 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65970/
> ---
> 
> (Updated March 7, 2018, 8:28 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the acknowledge call handler to take an rvalue reference
> for consistency. This also updates the handler to move the data into
> the outgoing message.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65970/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65971: Avoid a copy of the scheduler->executor message in the master.

[Meng Zhu]

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


Fix it, then Ship it!





src/master/master.cpp
Lines 5902-5905 (original), 5902-5905 (patched)


Also move this?



src/master/master.cpp
Line 5907 (original), 5907 (patched)


I found this call chain really confusing.
This function handles `FrameworkToExecutorMessage` by converting it into 
`scheduler::Call::Message` and pass it to `message()`. In `message()`, we just 
convert it back to `FrameworkToExecutorMessage` and send it. I wonder why don't 
we just send `FrameworkToExecutorMessage` straight away?

This seems to adhere to the pattern of converting internal message (used by 
v0) into Call and then process the call and generate other internal messages as 
needed. The confusing part here is that the message we receive happens to be 
the same to the message we send.

Do you think it is better to just forward it here instead of going through 
the unnecessary conversion?
Or add some comment regarding the detour?


- Meng Zhu


On March 7, 2018, 8:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65971/
> ---
> 
> (Updated March 7, 2018, 8:30 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This avoids a copy in both the v1 http and v0 message code paths.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65971/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65314: Removed code which is not used.

[Greg Mann]

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


Fix it, then Ship it!





src/common/http.hpp
Line 150 (original), 150 (patched)


In description: s/oboslete/obsolete/


- Greg Mann


On March 8, 2018, 12:26 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65314/
> ---
> 
> (Updated March 8, 2018, 12:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The introduction of the `ObjectApprovers` abstraction rendered the
> `AuthorizationAcceptor` class oboslete. After the refactor of the code
> the acceptor class was no longer used, nor were the helper functions
> built around it.
> 
> This patch removes that obsolete code.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
>   src/common/http.cpp 8d3a4ad63bd74e0313082bad3d89c21fbf54f256 
> 
> 
> Diff: https://reviews.apache.org/r/65314/diff/8/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 66002: Fixed the HTTP API path variables on Windows to use proper seperators.

[Andrew Schwartzmeyer]

> On March 9, 2018, 11:33 a.m., John Kordich wrote:
> > src/tests/files_tests.cpp
> > Line 338 (original), 338 (patched)
> > 
> >
> > Regarding this use of c_str():  I'm fairly certain this won't 
> > deallocate the result of path::join("1", "2") until after the call to 
> > stat() is complete, so the string should still be valid.  However, I'm not 
> > 100% certain, and I'd like someone else to judge this as well.
> > 
> > To be safe, I could allocate these paths outside of this function call, 
> > to guarantee that this string won't be deallocated.

Yeah, it's still in scope until `stat` has returned, so it's fine. Function 
stacks and all.


- Andrew


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


On March 9, 2018, 11:27 a.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66002/
> ---
> 
> (Updated March 9, 2018, 11:27 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the HTTP API path variables on Windows to use proper seperators.
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 5f92d2af722c3201200fa1d6a75dc0b87fdc6078 
>   src/tests/files_tests.cpp c703cae03345112715aeab83cb0a74abe3e12469 
> 
> 
> Diff: https://reviews.apache.org/r/66002/diff/1/
> 
> 
> Testing
> ---
> 
> Tested on both Windows and Linux, all tests pass, including the two newly 
> enabled tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 66002: Fixed the HTTP API path variables on Windows to use proper seperators.

[John Kordich via Review Board]

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




src/tests/files_tests.cpp
Line 338 (original), 338 (patched)


Regarding this use of c_str():  I'm fairly certain this won't deallocate 
the result of path::join("1", "2") until after the call to stat() is complete, 
so the string should still be valid.  However, I'm not 100% certain, and I'd 
like someone else to judge this as well.

To be safe, I could allocate these paths outside of this function call, to 
guarantee that this string won't be deallocated.


- John Kordich


On March 9, 2018, 7:27 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66002/
> ---
> 
> (Updated March 9, 2018, 7:27 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the HTTP API path variables on Windows to use proper seperators.
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 5f92d2af722c3201200fa1d6a75dc0b87fdc6078 
>   src/tests/files_tests.cpp c703cae03345112715aeab83cb0a74abe3e12469 
> 
> 
> Diff: https://reviews.apache.org/r/66002/diff/1/
> 
> 
> Testing
> ---
> 
> Tested on both Windows and Linux, all tests pass, including the two newly 
> enabled tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Review Request 66002: Fixed the HTTP API path variables on Windows to use proper seperators.

[John Kordich via Review Board]

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

Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, and 
Joseph Wu.


Repository: mesos


Description
---

Fixed the HTTP API path variables on Windows to use proper seperators.


Diffs
-

  src/files/files.cpp 5f92d2af722c3201200fa1d6a75dc0b87fdc6078 
  src/tests/files_tests.cpp c703cae03345112715aeab83cb0a74abe3e12469 


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


Testing
---

Tested on both Windows and Linux, all tests pass, including the two newly 
enabled tests on Windows.


Thanks,

John Kordich

Re: Review Request 65954: Add a gauge for how long agent recovery takes.

[James Peach]

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



This also needs to be documented in `docs/monitoring.md`.

- James Peach


On March 7, 2018, 11:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65954/
> ---
> 
> (Updated March 7, 2018, 11:20 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jason Lai, and James Peach.
> 
> 
> Bugs: MESOS-8609
> https://issues.apache.org/jira/browse/MESOS-8609
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new metric `slave/recover_secs` can be used to tell us how long
> Mesos agent needed to finish its recovery cycle. This is an important
> metric on agent machines which have a lot of completed executor
> sandboxes.
> 
> Note that the metric 1) will only be available after recovery succeeded
> and 2) never change its value across agent process lifecycle afterwards.
> 
> 
> Diffs
> -
> 
>   src/slave/metrics.hpp 3fc933ca65690d6fad63156398ad9c2c53789296 
>   src/slave/metrics.cpp 0eb2b59ed67e14e73b29d7592c239441df0008d5 
>   src/slave/slave.cpp e2facb3c15a2f907f6497c58a36842ed707f2c70 
> 
> 
> Diff: https://reviews.apache.org/r/65954/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 65954: Add a gauge for how long agent recovery takes.

[James Peach]

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




src/slave/metrics.hpp
Lines 45 (patched)


This doesn't need to be atomic. The reader will just read either the old or 
new values and it doesn't matter which it gets.



src/slave/metrics.cpp
Lines 257 (patched)


My suggestion for the metric name is:
```
slave/recovery_time_secs
```



src/slave/metrics.cpp
Lines 259 (patched)


I don't know that I like the idea of a metric that is absent and then 
present. I'd prefer that we just published a `0.0` until recovert is complete.

Suggest we keep the recovery timestamp in the `Slave` and just publish that.



src/slave/slave.cpp
Lines 7322 (patched)


Since the gauge is being published in seconds, you need to use 
`Duration::secs` to convert.


- James Peach


On March 7, 2018, 11:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65954/
> ---
> 
> (Updated March 7, 2018, 11:20 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jason Lai, and James Peach.
> 
> 
> Bugs: MESOS-8609
> https://issues.apache.org/jira/browse/MESOS-8609
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new metric `slave/recover_secs` can be used to tell us how long
> Mesos agent needed to finish its recovery cycle. This is an important
> metric on agent machines which have a lot of completed executor
> sandboxes.
> 
> Note that the metric 1) will only be available after recovery succeeded
> and 2) never change its value across agent process lifecycle afterwards.
> 
> 
> Diffs
> -
> 
>   src/slave/metrics.hpp 3fc933ca65690d6fad63156398ad9c2c53789296 
>   src/slave/metrics.cpp 0eb2b59ed67e14e73b29d7592c239441df0008d5 
>   src/slave/slave.cpp e2facb3c15a2f907f6497c58a36842ed707f2c70 
> 
> 
> Diff: https://reviews.apache.org/r/65954/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 65938: Replaced "re-register" terminology with "reregister".

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 6, 2018, 11:52 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65938/
> ---
> 
> (Updated March 6, 2018, 11:52 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-6128
> https://issues.apache.org/jira/browse/MESOS-6128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We sometimes use "re-register" in comments and elsewhere we
> use "reregister". We should pick the "reregister" form and
> use it consistently.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 20bfe210d1b9b027a3be0712fe71c560b177d061 
>   docs/agent-recovery.md 8df72cb3adfec9f52429b88c19c437c6fe3d5fd9 
>   docs/app-framework-development-guide.md 
> e31bffc78daffa9f82ecf5361f91281354f9e466 
>   docs/configuration/agent.md 71e5a86cbed6cd646c398800889308bc2912083f 
>   docs/configuration/master.md 7b2bb98c43e7f419e6bbe28fdb0f3d1946557165 
>   docs/high-availability-framework-guide.md 
> f2467735bfc4c47c21d108c50f74aea4d302e252 
>   docs/high-availability.md 1bad0ee6ddeebbf47dbc0c202c28cdec3000fb75 
>   docs/maintenance.md 88434e843279626ef89c61c3f70cc9da54b3eee8 
>   docs/monitoring.md 7ad15b4f821108083b5f9f4381c33d5f465d361b 
>   docs/operator-http-api.md 64110f03ad3158765f8b928a68d004e1f296d4c8 
>   docs/reconciliation.md 473e3e2ad08ae0154c3d84cd72de076a3922638e 
>   docs/roles.md ddea114baed0e378b43fd7a9462787cf0689216c 
>   docs/task-state-reasons.md 5f7f8d2eeb91d47c44bdf665fe31eb474933c2de 
>   include/mesos/authorizer/acls.proto 
> 086274388ace3a60c68cca1a37507af3ec429bf8 
>   include/mesos/executor.hpp 68cba9e3c411eb9f396b49adcc9d71137a5d948c 
>   include/mesos/master/master.proto f40caa2a6338ff58a325444cfb72cd24f2c99472 
>   include/mesos/mesos.proto 2c5ae4cd8d9cfaabd42a9e94ce94cd73bfacc6eb 
>   include/mesos/scheduler.hpp 423235a90166e71866a88baa3bf16ba551010c5c 
>   include/mesos/scheduler/scheduler.proto 
> 7a90355c3d1a362ebb4fdb63bfe1fba8aca60d5b 
>   include/mesos/v1/master/master.proto 
> 67c9560eaa7bd6b20b73af07f7666f0d98c4453d 
>   include/mesos/v1/mesos.proto 67080ed2907a2fee53dab4802dce0aeac3424412 
>   site/source/blog/2013-10-23-slave-recovery-in-apache-mesos.md 
> 5b604f591d20b08cf4329f7503eaa4d137e5ed85 
>   site/source/blog/2014-06-12-mesos-0-19-0-released.md 
> 2d2a3db21580422761cd1895b5aab5c894c7e72f 
>   site/source/blog/2017-09-18-mesos-1-4-0-released.md 
> fcdc2e426ef95e4ee90f6a637ac083aa6d0f2cf7 
>   site/source/blog/2017-12-07-performance-working-group-progress-report.md 
> b7f5a8786953f54b63704e09c23528003052f695 
>   src/exec/exec.cpp 0c76a3f1d05b3ff93b04a2bf4ac6a9f433dd1d19 
>   src/java/src/org/apache/mesos/Executor.java 
> 078c0c7ea3ad68e9b4b40802c3d6b94084cbb69b 
>   src/java/src/org/apache/mesos/Scheduler.java 
> cb5a65872fa4bbd711c86af258e58aa7b33777b8 
>   src/master/allocator/mesos/hierarchical.cpp 
> e0ab8360c282092b141a12e8628f3d08039a0b69 
>   src/master/constants.hpp 17092ea1b43a3fa86fbbcb4dff6b24922054ae12 
>   src/master/flags.cpp 2f4aa697c2cb68cc3a23d42ef5f86cff95749b23 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
>   src/python/executor/src/mesos/executor/proxy_executor.cpp 
> 2462ccb3be1f7e0078448bfe0e1dc577c859422f 
>   src/python/interface/src/mesos/interface/__init__.py 
> 048eb6438d204aa20d71a8d59f516f06934f2d99 
>   src/sched/sched.cpp 81aa0d7d34753be2bac49753be1ab3be21746d5d 
>   src/slave/constants.hpp b79c084f4952affec531f29797479f3e702a66b6 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> f1d377eb911fb6a0ca3a4749a0e00d5166f35129 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> df240bcff9e53faab275ac41ba3221789467dca6 
>   src/slave/flags.cpp 943aaaf58b5f36555f0902019b8c5c6522ab7afc 
>   src/slave/slave.hpp f161369732932ec99abbf635f91c88425744b3a9 
>   src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219 
>   src/slave/task_status_update_manager.cpp 
> 24d803d5ba0dab5c0ca5316cf2affedba3a6e65e 
>   src/tests/api_tests.cpp 9c172f751b11b3b50f23253d212c2eb603da9c2c 
>   src/tests/authentication_tests.cpp 3c6124f641b64cb6f67cb1605958e12151b91456 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 85e6f8739dd2d6f9119e30f13f681cf4b8dc45ed 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> 9123cbe50e4a3417dc4f205092a436d7230c7414 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 488051dbf85a9d0baf5f61b8c0d28aa14dad9427 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> 0969cf545254c6cadfd69fa0f4f4f087f07af4b7 
>   

Re: Review Request 65994: Made the master forward operation status updates to the schedulers.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65993, 65994]

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 March 8, 2018, 2:07 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65994/
> ---
> 
> (Updated March 8, 2018, 2:07 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-8189
> https://issues.apache.org/jira/browse/MESOS-8189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the master forward operation status updates to the schedulers.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65994/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65936: Changed flags to CreateFile to support Windows symlink path resolution.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On March 8, 2018, 3:21 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65936/
> ---
> 
> (Updated March 8, 2018, 3:21 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jeff Coffler, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-8646
> https://issues.apache.org/jira/browse/MESOS-8646
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed flags to CreateFile to support Windows symlink path resolution.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp 
> 858b3c7c27e95b9cf46a7457c1aaa05a0e19188d 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> b84f1ae17246d94947538efeaf504a2cd247f20e 
> 
> 
> Diff: https://reviews.apache.org/r/65936/diff/3/
> 
> 
> Testing
> ---
> 
> I encountered a permissions error stating "The process cannot access the file 
> because it is being used by another process." when attempting to download 
> stderr/stdout from a task running on a Windows agent, or when attempting to 
> download the mesos agent log on a Windows agent. 
> 
> After some very helpful information from Andy and Akash, I made this change 
> and confirmed it fixed this issue with the task/agent logs.
> 
> I also ran the automated tests on Windows and all tests pass.
> 
> I did not test this on Linux, but because this file is compiled only on 
> Windows, it shouldn't be necessary.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66001 was successfully built and tested.

Reviews applied: `['66000', '66001']`

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

- Mesos Reviewbot Windows


On March 9, 2018, 9:04 a.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66001/
> ---
> 
> (Updated March 9, 2018, 9:04 a.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New Flags for disk/xfs isolator
> - xfs_disk_hard_limit_offset_pct - creates a gap between
> - xfs_disk_hard_limit_offset - overrides _pct creates an offset between
>   soft and hard limits of a container.
> - xfs_kill_after_grace_period - will cause a container to be killed if
>   the soft limit has been breached after the `xfs_quota` configured
>   grace period has ended.
> - xfs_kill_check_interval - Period of delay between which the isolator
>   will check for violations of the soft limit.
> 
> Functionality
> - Add head room to the hard limit as a percentage or specified amount
>   for each container. This is specified at a flag level and not
>   customizable on a per container basis.
> - Provide the ability for an application to be killed after the
>   configured grace period for projects is violated.
> - Add an interval between which the watcher will check for violations.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 07e68a777aefba4dd35066f2eb207bba7f199d83 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 8d9f8f846866f9de377c59cb7fb311041283ba70 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> e034133629a9c1cf58b776f8da2a93421332cee0 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> 2708524add1ff693b616d4fb241c4a0a3070520b 
>   src/slave/flags.hpp 0c67bf214ceb93ae7ff088bec2648fa26ddac59e 
>   src/slave/flags.cpp 943aaaf58b5f36555f0902019b8c5c6522ab7afc 
> 
> 
> Diff: https://reviews.apache.org/r/66001/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 65972: Avoid copying scheduler->executor message data in the master.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On March 7, 2018, 8:34 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65972/
> ---
> 
> (Updated March 7, 2018, 8:34 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-8628
> https://issues.apache.org/jira/browse/MESOS-8628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This avoids a copy of the scheduler->executor message for the
> v0 message path in the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65972/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 66000: Update enforce_container_disk_quota documentation to include disk/xfs.

[Harold Dost]

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

Review request for mesos and James Peach.


Repository: mesos


Description
---

Update enforce_container_disk_quota documentation to include disk/xfs.


Diffs
-

  src/slave/flags.cpp 943aaaf58b5f36555f0902019b8c5c6522ab7afc 


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


Testing
---


Thanks,

Harold Dost

Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.

[Harold Dost]

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

Review request for mesos and James Peach.


Repository: mesos


Description
---

New Flags for disk/xfs isolator
- xfs_disk_hard_limit_offset_pct - creates a gap between
- xfs_disk_hard_limit_offset - overrides _pct creates an offset between
  soft and hard limits of a container.
- xfs_kill_after_grace_period - will cause a container to be killed if
  the soft limit has been breached after the `xfs_quota` configured
  grace period has ended.
- xfs_kill_check_interval - Period of delay between which the isolator
  will check for violations of the soft limit.

Functionality
- Add head room to the hard limit as a percentage or specified amount
  for each container. This is specified at a flag level and not
  customizable on a per container basis.
- Provide the ability for an application to be killed after the
  configured grace period for projects is violated.
- Add an interval between which the watcher will check for violations.


Diffs
-

  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
07e68a777aefba4dd35066f2eb207bba7f199d83 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
8d9f8f846866f9de377c59cb7fb311041283ba70 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
e034133629a9c1cf58b776f8da2a93421332cee0 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
2708524add1ff693b616d4fb241c4a0a3070520b 
  src/slave/flags.hpp 0c67bf214ceb93ae7ff088bec2648fa26ddac59e 
  src/slave/flags.cpp 943aaaf58b5f36555f0902019b8c5c6522ab7afc 


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


Testing
---


Thanks,

Harold Dost

Re: Review Request 65962: Avoided copying `Owned` pointers in the default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65692, 65693, 65694, 65695, 65962]

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 March 8, 2018, 10 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65962/
> ---
> 
> (Updated March 8, 2018, 10 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Owned` pointers are copied in multiple places of the default executor.
> This violates the semantic of owned pointers and works only because
> `Owned` is currently implemented with `shared_ptr`, it would otherwise
> lead to double-freeing the pointers.
> 
> This patch changes those places to use references to the original
> `Owned` objects or raw pointers instead of copies.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 906836f3b8e0af79d7c61f90fd8a95f193b26e84 
> 
> 
> Diff: https://reviews.apache.org/r/65962/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*Default*"` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

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

(Updated March 9, 2018, 10:08 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and 
Zhitao Li.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:

https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 


Diff: https://reviews.apache.org/r/59987/diff/7/

Changes: https://reviews.apache.org/r/59987/diff/6-7/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 391-487 (patched)
> > 
> >
> > It looks like we can avoid this logic since protobuf's JSON conversion 
> > allows the protobuf key types (bool, integers, strings) to be converted 
> > from JSON strings. This means we could just recurse for both key and value 
> > here:
> > 
> > ```
> > // Some comment explaining what map is equivalent to with
> > // a reference to the google documentation.
> > google::protobuf::Message* entry =
> >   reflection->AddMessage(message, field);
> > 
> > const google::protobuf::FieldDescriptor* key_field =
> >   entry->GetDescriptor()->FindFieldByNumber(1);
> >   
> > // Maybe passing just 'key' works due to implicit 
> > construction?
> > // TODO(...): This copies the key, consider avoiding this.
> > Try apply =
> >   boost::apply_visitor(Parser(entry, key_field), 
> > JSON::String(key));
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> >   
> > const google::protobuf::FieldDescriptor* value_field =
> >   entry->GetDescriptor()->FindFieldByNumber(2);
> >   
> > Try apply =
> >   boost::apply_visitor(Parser(entry, value_field), value);
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> > ```
> > 
> > Now, to make this simplification work, we need to update our JSON 
> > conversion in a separate patch to allow bools and integers to be parsed 
> > from JSON strings to match google's logic for conversion:
> > 
> > 
> > https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
> > 
> > Documentation here: 
> > https://developers.google.com/protocol-buffers/docs/proto3#json
> 
> Qian Zhang wrote:
> > // Maybe passing just 'key' works due to implicit construction?
> 
> We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` 
> because it cannot pass compilation:
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'std::__1::basic_string'
> ```
> or
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'JSON::String'
> ```
> That is because `std::string` and `JSON::String` do not have a member 
> `apply_visitor`, so I think we have to use `JSON::Value` like this:
> ```
> google::protobuf::Message* entry =
>   reflection->AddMessage(message, field);
> 
> const google::protobuf::FieldDescriptor* key_field =
>   entry->GetDescriptor()->FindFieldByNumber(1);
> 
> JSON::Value key(name);
> 
> Try apply =
>   boost::apply_visitor(Parser(entry, key_field), key);
> 
> if (apply.isError()) {
>   return Error(apply.error());
> }
> 
> const google::protobuf::FieldDescriptor* value_field =
>   entry->GetDescriptor()->FindFieldByNumber(2);
> 
> apply = boost::apply_visitor(Parser(entry, value_field), 
> value);
> if (apply.isError()) {
>   return Error(apply.error());
> }
> ```
> 
> > we need to update our JSON conversion in a separate patch to allow 
> bools and integers to be parsed from JSON strings to match google's logic for 
> conversion
> 
> Did you mean we should improve the method below by adding more cases for 
> converting `JSON::String` to bools and integers?
> ```
>   Try operator()(const JSON::String& string) const
>   {
> switch (field->type()) {
>   case google::protobuf::FieldDescriptor::TYPE_STRING:
>   ...
> ```
> If so, then that means moving the code here (see below) into the above 
> method. But I think those code are specific for map support, however 
> `Try operator()(const JSON::String& string) const` is a generic 
> method for JSON string conversion, so I would still like to keep those code 
> as where they are now in this patch (i.e., the code path to handle map).
> ```
>   case google::protobuf::FieldDescriptor::TYPE_INT64:
>   case google::protobuf::FieldDescriptor::TYPE_SINT64:
>   case google::protobuf::FieldDescriptor::TYPE_SFIXED64:
>   {
> 

Re: Review Request 65313: Refactored authorization logic in the agent.

[Alexander Rojas]

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

(Updated March 9, 2018, 2:06 p.m.)


Review request for mesos, Benjamin Hindman and Greg Mann.


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


Repository: mesos


Description
---

This patch makes uses of the new `ObjectApprovers` class which greatly
simplifies the logic for constructing and using authorization.


Diffs (updated)
-

  src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b 
  src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b 
  src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219 


Diff: https://reviews.apache.org/r/65313/diff/7/

Changes: https://reviews.apache.org/r/65313/diff/6-7/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 65313: Refactored authorization logic in the agent.

[Alexander Rojas]

> On March 9, 2018, 6:30 a.m., Greg Mann wrote:
> > src/slave/http.cpp
> > Lines 2641-2665 (original), 2431-2469 (patched)
> > 
> >
> > Why did you opt for a new templated method instead of the pre-existing 
> > lambda, here and elsewhere? Is it necessary because we pass the action as a 
> > template parameter?

It has to do more with the `Approvers::approved()` being parametrize, which 
means that the type needs to be resolved on compilation time, so this is a neat 
trick for that.


- Alexander


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


On March 8, 2018, 1:26 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65313/
> ---
> 
> (Updated March 8, 2018, 1:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes uses of the new `ObjectApprovers` class which greatly
> simplifies the logic for constructing and using authorization.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp c33adeb2ddd36e8be1324b3ddb1401bdf7e4e80b 
>   src/slave/http.cpp 7d7fa2b4ec2e1f8f65c5264ce72590d0d8195b9b 
>   src/slave/slave.cpp 8cb6899bf15fb697c3cb2784f63b7c2d5729d219 
> 
> 
> Diff: https://reviews.apache.org/r/65313/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>