Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[Mesos Reviewbot Windows]

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



FAIL: The file 'C:DCOSmesosbuild-outputlogsapply-review-63270-stdout.log' 
already exists.

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

- Mesos Reviewbot Windows


On Oct. 24, 2017, 11:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated Oct. 24, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about setting O_CLOEXEC on the pipe file
> descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 100e3bbda543d87808da9ff6bea42da5099ea8c5 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60620, 60621, 60622, 60623, 60624, 60626, 60628, 63253]

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 Oct. 24, 2017, 6:04 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63253/
> ---
> 
> (Updated Oct. 24, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017)
> for proper operation on Windows. Changed to use new URI methods
> introduced as part of MESOS-6705, and used powershell on Windows
> to replace "test" command on Linux.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 5078bd4d70698f5cbd14c971fcecfd58f8467a04 
> 
> 
> Diff: https://reviews.apache.org/r/63253/diff/1/
> 
> 
> Testing
> ---
> 
> Ran mesos-tests on both Windows and Linux with no errors.
> 
> Specifically ran mesos-tests with 
> `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the 
> modified tests ran properly.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 62878: Sent CheckpointResourcesMessage only when reregister with an old master.

[Greg Mann]

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




src/master/master.cpp
Lines 6321-6355 (patched)


As discussed offline, I think it makes sense to put this block in 
`reconcileKnownAgent`, as you previously suggested. I see no reason not to do 
it in this patch, but one of us could follow up with another patch as well.


- Greg Mann


On Oct. 17, 2017, 11:24 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62878/
> ---
> 
> (Updated Oct. 17, 2017, 11:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Joseph Wu, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-8058
> https://issues.apache.org/jira/browse/MESOS-8058
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> No need for sending checkpoint resources message to the agent if the
> master does not have state about the agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3603878f02ae3dba82811a4a5770dd21ec790ef6 
> 
> 
> Diff: https://reviews.apache.org/r/62878/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63256: Added a master validation 'resource::validateSingleResourceProvider'.

[Greg Mann]

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




src/master/validation.cpp
Lines 816 (patched)


"Some resources have a resource provider and some don't"

Here and below.



src/master/validation.cpp
Lines 821 (patched)


`.get()` is not needed here.



src/tests/master_validation_tests.cpp
Lines 285 (patched)


I think this might read better if you enclose each test case in a `{}` 
scope and declare a new `Resources` object in each scope.


- Greg Mann


On Oct. 24, 2017, 6:05 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63256/
> ---
> 
> (Updated Oct. 24, 2017, 6:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This validates that all resources are from the same resource provider.
> We don't allow an offer operation to be operating on resources from
> different resource providers.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585 
>   src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb 
>   src/tests/master_validation_tests.cpp 
> 7da1be5233444aded64263d4a763fe2967459042 
> 
> 
> Diff: https://reviews.apache.org/r/63256/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63264: Added Meng Zhu to contributors.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63264 was successfully built and tested.

Reviews applied: `['63264']`

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

- Mesos Reviewbot Windows


On Oct. 24, 2017, 3:21 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63264/
> ---
> 
> (Updated Oct. 24, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Meng Zhu to contributors.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 63bee83835297c3beb6edbd24e6ed027e457f3bd 
> 
> 
> Diff: https://reviews.apache.org/r/63264/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 63270: Updated `os::pipe()` to always return O_CLOEXEC descriptors.

[James Peach]

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

(Updated Oct. 25, 2017, 12:04 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and Jie 
Yu.


Changes
---

Added FreeBSD.


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


Repository: mesos


Description (updated)
---

Updated `os::pipe()` to always return O_CLOEXEC descriptors,
atomically if we are on Linux or FreeBSD and the `pipe2(2)`
sytem call is available.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/posix/pipe.hpp 
ac76224ff8fa1cc535c78731580e77a27967136a 


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

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 63232: Converted SlaveInfo resources from the registry during master recovery.

[Benjamin Mahler]

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


Fix it, then Ship it!




What goes wrong with this issue? Would be good to clarify that in the 
description.

Have you considered a test?


src/master/master.cpp
Lines 1701-1702 (patched)


Can you comment on why this is here, for posterity? Something similar to 
your review description would be helpful!


- Benjamin Mahler


On Oct. 23, 2017, 11:56 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63232/
> ---
> 
> (Updated Oct. 23, 2017, 11:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Greg Mann.
> 
> 
> Bugs: MESOS-7851
> https://issues.apache.org/jira/browse/MESOS-7851
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The intention is for us to work with resources in the 
> `post-reservation-refinement` format
> in the master memory, and store them in the master registry or agent 
> checkpoint in
> the `pre` format in order to support downgrades. In this case, we correctly 
> store the `pre`
> format resources in the master registry, but we don't convert them back when 
> we read it out
> of the registry. This patch converts the resources back to `post` when we 
> read from the registry.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 4b76648b4887724e166ff95e002ee26c8bfef006 
> 
> 
> Diff: https://reviews.apache.org/r/63232/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 63257: Added a few test helpers for creating disk resources.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [63001, 62903, 63094, 63104, 63254, 63255, 63256, 63257]

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 Oct. 24, 2017, 6:06 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63257/
> ---
> 
> (Updated Oct. 24, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a few test helpers for creating disk resources.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 4b61f2dbd67a5e8a65a6655519c8f03e5d6c6954 
> 
> 
> Diff: https://reviews.apache.org/r/63257/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

[James Peach]

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

Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.


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


Repository: mesos


Description
---

Since the containerizer launch depends on the inherited pipe to signal
the forked child, be explicit about setting O_CLOEXEC on the pipe file
descriptors. Make sure to close the pipe on the error paths.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
100e3bbda543d87808da9ff6bea42da5099ea8c5 


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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Review Request 63270: Updated `os::pipe()` to always return O_CLOEXEC descriptors.

[James Peach]

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

Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.


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


Repository: mesos


Description
---

Updated `os::pipe()` to always return O_CLOEXEC descriptors, atomically
if we are on Linux and the `pipe2(2)` sytem call is available.


Diffs
-

  3rdparty/stout/include/stout/os/posix/pipe.hpp 
ac76224ff8fa1cc535c78731580e77a27967136a 
  README.md e3ca837515afe3daa00996562b3f86c77cce0909 
  docs/advanced-contribution.md 6af24ccf32745dada7bb9b637a7ccd37b4769c72 
  docs/beginner-contribution.md 471f5dd0da30d921cc3b29484d2b02f164f8ca75 


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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 63264: Added Meng Zhu to contributors.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Oct. 24, 2017, 3:21 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63264/
> ---
> 
> (Updated Oct. 24, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Meng Zhu to contributors.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 63bee83835297c3beb6edbd24e6ed027e457f3bd 
> 
> 
> Diff: https://reviews.apache.org/r/63264/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 63264: Added Meng Zhu to contributors.

[Meng Zhu]

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

Review request for mesos, Chun-Hung Hsiao and Gilbert Song.


Repository: mesos


Description
---

Added Meng Zhu to contributors.


Diffs
-

  docs/contributors.yaml 63bee83835297c3beb6edbd24e6ed027e457f3bd 


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


Testing
---


Thanks,

Meng Zhu

Re: Review Request 63174: Added a benchmark for agent reregistration during master failover.

[Benjamin Mahler]

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



A couple of suggestions for speeding up the benchmark overhead:

(1) Upgrade protobuf to 3.4.x, this comes with move support and rvalue setters 
for fields. Which will avoid some copies in the benchmark code and improve 
performance elsewhere too :) In the interim, you could manually use `Swap(T*)` 
but it means we'd probably want to re-write the code once move support is 
available (so that doesn't seem like a good option).

(2) You could try using an arena for the test fixture, although I don't know if 
it's worth the complexity. Probably just reducing copying is simpler.

(3) We can avoid re-parsing resources for each task and agent.


src/tests/master_benchmarks.cpp
Lines 63 (patched)


Can you avoid parsing resources for each agent?



src/tests/master_benchmarks.cpp
Lines 84 (patched)


Can you avoid parsing resources for every task?



src/tests/master_benchmarks.cpp
Lines 91-92 (patched)


Code written this way is nice because it will automatically benefit from 
move support when we upgrade protobuf to 3.4.x. :)

Maybe you can write more of the test in such a manner that it would benefit 
from an upgrade to 3.4.x? I would be happy to review a 3.4.x upgrade since we 
need it for other performance improvements. We can see who wants to pick that 
up, I think Dmitry might be interested.



src/tests/master_benchmarks.cpp
Lines 139-140 (patched)


Here's an example of where you could move into `message.frameworks` if you 
upgrade to protobuf 3.4.x:

```
message.mutable_frameworks()->Add(createFrameworkInfo(frameworkId));
```

Alternatively, pre-3.4.x, you can swap:

```
message.add_frameworks()->Swap((frameworkId));

// maybe you need to do:

FrameworkInfo f = createFrameworkInfo(frameworkId);
message.add_frameworks()->Swap();
```



src/tests/master_benchmarks.cpp
Lines 143-147 (patched)


Ditto copying here.



src/tests/master_benchmarks.cpp
Lines 163-167 (patched)


Ditto copying here and elsewhere.



src/tests/master_benchmarks.cpp
Lines 241-243 (patched)


Comment about why you're using the replicated log here?



src/tests/master_benchmarks.cpp
Lines 261 (patched)


I'm a little concerned about this pattern, because if the test were to fail 
an assertion, the process would be destructed without terminating / waiting on 
it.

Can you use a wrapper around the process that terminates and waits?

Alternatively, if we had a SCOPE_EXIT { ... } abstraction (I had a review 
but never committed it), we could just do:

```
SCOPE_EXIT { process::terminate(pid); wait(pid); };
```

E.g. 
https://github.com/facebook/folly/blob/v2017.10.23.00/folly/ScopeGuard.h#L285-L287


- Benjamin Mahler


On Oct. 24, 2017, 6:05 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63174/
> ---
> 
> (Updated Oct. 24, 2017, 6:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Dmitry Zhuk, and Ilya Pronin.
> 
> 
> Bugs: MESOS-8098
> https://issues.apache.org/jira/browse/MESOS-8098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current benchmark is very simple: without framework involvement and 
> without agent retries but it's possible to add a number of others so I am 
> creating a new file for them.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/master_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63174/diff/2/
> 
> 
> Testing
> ---
> 
> Benchmark based off 
> https://github.com/apache/mesos/commit/41193181d6b75eeecae2729bf98007d9318e351a
>  (close to current HEAD).
> 
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 11.188008209secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (22404 ms)
> [ RUN  ] 
> 

Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Oct. 24, 2017, 11:04 a.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63253/
> ---
> 
> (Updated Oct. 24, 2017, 11:04 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017)
> for proper operation on Windows. Changed to use new URI methods
> introduced as part of MESOS-6705, and used powershell on Windows
> to replace "test" command on Linux.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 5078bd4d70698f5cbd14c971fcecfd58f8467a04 
> 
> 
> Diff: https://reviews.apache.org/r/63253/diff/1/
> 
> 
> Testing
> ---
> 
> Ran mesos-tests on both Windows and Linux with no errors.
> 
> Specifically ran mesos-tests with 
> `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the 
> modified tests ran properly.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 63174: Added a benchmark for agent reregistration during master failover.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63174 was successfully built and tested.

Reviews applied: `['63174']`

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

- Mesos Reviewbot Windows


On Oct. 24, 2017, 2:05 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63174/
> ---
> 
> (Updated Oct. 24, 2017, 2:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Dmitry Zhuk, and Ilya Pronin.
> 
> 
> Bugs: MESOS-8098
> https://issues.apache.org/jira/browse/MESOS-8098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current benchmark is very simple: without framework involvement and 
> without agent retries but it's possible to add a number of others so I am 
> creating a new file for them.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/master_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63174/diff/2/
> 
> 
> Testing
> ---
> 
> Benchmark based off 
> https://github.com/apache/mesos/commit/41193181d6b75eeecae2729bf98007d9318e351a
>  (close to current HEAD).
> 
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 11.188008209secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (22404 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 20.868372615secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (37981 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 15.354579251secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (33766 ms)
> [--] 3 tests from 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (94151 ms total)
> 
> 
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 11.045441129secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (19959 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 21.324309077secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (38490 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 14.68607521secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (32073 ms)
> [--] 3 tests from 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (90523 ms total)
> 
> ```
> 
> Benchmark based off 
> https://github.com/apache/mesos/commit/d9c90bf1d9c8b3a7dcc47be0cb773efff57cfb9d
>  (before https://issues.apache.org/jira/browse/MESOS-7713 was merged)
> 
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 23.217901878secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (38327 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 46.158610597secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (75280 ms)
> [ RUN  ] 
> 

Re: Review Request 63174: Added a benchmark for agent reregistration during master failover.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [63174]

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 Oct. 24, 2017, 6:05 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63174/
> ---
> 
> (Updated Oct. 24, 2017, 6:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Dmitry Zhuk, and Ilya Pronin.
> 
> 
> Bugs: MESOS-8098
> https://issues.apache.org/jira/browse/MESOS-8098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current benchmark is very simple: without framework involvement and 
> without agent retries but it's possible to add a number of others so I am 
> creating a new file for them.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/master_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63174/diff/2/
> 
> 
> Testing
> ---
> 
> Benchmark based off 
> https://github.com/apache/mesos/commit/41193181d6b75eeecae2729bf98007d9318e351a
>  (close to current HEAD).
> 
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 11.188008209secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (22404 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 20.868372615secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (37981 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 15.354579251secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (33766 ms)
> [--] 3 tests from 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (94151 ms total)
> 
> 
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 11.045441129secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (19959 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 21.324309077secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (38490 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 14.68607521secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (32073 ms)
> [--] 3 tests from 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (90523 ms total)
> 
> ```
> 
> Benchmark based off 
> https://github.com/apache/mesos/commit/d9c90bf1d9c8b3a7dcc47be0cb773efff57cfb9d
>  (before https://issues.apache.org/jira/browse/MESOS-7713 was merged)
> 
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 23.217901878secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (38327 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 46.158610597secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (75280 ms)
> [ RUN  ] 
> 

Re: Review Request 63245: Ported and enabled Os Tests on Windows.

[Andrew Schwartzmeyer]

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



This review request should really be at least three separate patches. THere is 
work to be done to fix several of these tests, and right now they're all mixed 
together.


3rdparty/stout/include/stout/os.hpp
Line 120 (original), 120 (patched)


We can't do this, it's nonsense. `PATH` does not make sense as an 
environment variable here, as, like you said, there is no equivalent to 
`LD_LIBRARY_PATH` on Windows.



3rdparty/stout/include/stout/os/windows/stat.hpp
Line 112 (original), 112 (patched)


Why was this change made?



3rdparty/stout/include/stout/windows/os.hpp
Lines 390-392 (patched)


This should be an `Error`, not a `WindowsError`, as the latter is reserved 
for OS errors (explicitly, its implementation calls `GetLastError()` and 
retrieves the Windows error message, none of which should happen here).

While the POSIX implementation does not explicitly check for `duration < 
0`, since the system call itself fails, and that failure is returned, I agree 
that it's reasonable here to return an `Error`. However, a note should be added 
to the function as to why we do that (specificically, so that the `os::sleep` 
API remains consistent).



3rdparty/stout/tests/os_tests.cpp
Lines 213-215 (original), 213-215 (patched)


If this is resolved, this comment needs to be removed.



3rdparty/stout/tests/os_tests.cpp
Lines 232-233 (original)


Removing this test code does not resolve this issue. In fact, it's reducing 
test coverage, as there is clearly a bug here highlighted by this failing test.

Instead, I agree with you here:

> If you are supposed to be able to test the size of a non-link file in
the context of not-following-a-link, the program should return the
size of the non-link file instead of returning an error upon determining the
nature of the path as not being a link file path.

So the correct solution is to fix `os::stat::size` on Windows to work 
correctly, at which point this test can be enabled.



3rdparty/stout/tests/os_tests.cpp
Lines 274-275 (original), 272-273 (patched)


If this is working now, the TODO here should be removed.



3rdparty/stout/tests/os_tests.cpp
Lines 885-925 (original), 883-923 (patched)


Having changed `ldLibraryPath` above to `PATH`, this just potentially broke 
numerous things by overriding the system's `PATH` (hopefully just for this 
process, but I digress, it's incorrect).

This test sh ould probably _never_ be enabled on Windows, as there is 
simply no equivalent to `LD_LIBRARY_PATH`. Windows loads libraries from a 
static set of paths that cannot be changed.

Instead of enabling this test, it should be explicitly `#ifdef'd` out with 
an explanation for Windows that the API simply cannot be ported.



3rdparty/stout/tests/os_tests.cpp
Line 1023 (original), 1021 (patched)


This is incorrect. This test is explicitly asserting that the `testLink` 
resolved to the `testFile`. Changing the test is wrong.

Instead, there is a set of tasks to fix `realpath` on Windows, including 
MESOS-6735, MESOS-5881, and even MESOS-7604.

For now, we'll leave fixing `realpath` separate to fixing these other tests.


- Andrew Schwartzmeyer


On Oct. 24, 2017, 2:30 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63245/
> ---
> 
> (Updated Oct. 24, 2017, 2:30 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3441
> https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> TEST_F(OsTest, Libraries)
> The test failed because no environmental variable was provided for
> Windows. I used "PATH" as environmental variable on Windows since
> there's no default variable where the linker should look into while
> linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on 
> Unix/Linux)
> 
> TEST_F(OsTest, Sleep)
> The test hanged because of the last assertion:
> ASSERT_ERROR(os::sleep(Milliseconds(-10)));
> Apparently, it didn't handle a negative sleep duration. By simply
> returning an error value when the duration is negative the problem was
> solved.
> 
> 

Re: Review Request 62877: Added RESOURCE_PROVIDER agent capability.

[Greg Mann]

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



I noticed that we have an agent capabilities test here: 
https://github.com/apache/mesos/blob/d5630f3412dab3b3464f59c57b7526068ebbcb96/src/tests/protobuf_utils_tests.cpp#L278-L292

I'm not convinced that test is necessary, given how simple the constructor is 
and the fact that other tests exercise the MULTI_ROLE capability and would fail 
if the constructor doesn't work.
I just wanted to give you a heads up on that test in case you thought it was 
beneficial and wanted to add your new capability to it.

- Greg Mann


On Oct. 17, 2017, 11:24 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62877/
> ---
> 
> (Updated Oct. 17, 2017, 11:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8071
> https://issues.apache.org/jira/browse/MESOS-8071
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added RESOURCE_PROVIDER agent capability.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ba87339dbe341f4d16ceea74adc09647a3c07f32 
>   include/mesos/v1/mesos.proto a6d662fb26aa4f78ef20ffe6e013f7a45f7f8c21 
>   src/common/protobuf_utils.hpp ff0fd01b7a20f597ec6cd916f0bd9c41baa5fd51 
>   src/common/protobuf_utils.cpp 04f44f6f63e431c17ec67e234c8da58e7945294d 
> 
> 
> Diff: https://reviews.apache.org/r/62877/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63239: Ported and enabled Flag Tests on Windows.

[Andrew Schwartzmeyer]

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




3rdparty/stout/tests/flags_tests.cpp
Lines 229-231 (original), 229-231 (patched)


If it works out of the box now, this comment should be removed.



3rdparty/stout/tests/flags_tests.cpp
Lines 527-528 (original), 527-528 (patched)


Ditto.


- Andrew Schwartzmeyer


On Oct. 24, 2017, 12:57 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63239/
> ---
> 
> (Updated Oct. 24, 2017, 12:57 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3437
> https://issues.apache.org/jira/browse/MESOS-3437
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> They worked out of the box.
> Apparently the issues MESOS-5880 and MESOS-5937 which were causing the tests 
> to fail were solved.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/flags_tests.cpp 
> 88c8ee57c2bf328251513761498ef98d774be006 
> 
> 
> Diff: https://reviews.apache.org/r/63239/diff/1/
> 
> 
> Testing
> ---
> 
> Modified Tests:
> TEST(FlagsTest, LoadFromEnvironment)
> TEST(FlagsTest, DuplicatesFromEnvironment)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Re: Review Request 63238: Ported and enabled Path Tests on Windows.

[Andrew Schwartzmeyer]

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




3rdparty/stout/tests/path_tests.cpp
Lines 40-61 (original), 43-90 (patched)


Instead of using an `#ifdef` here, we can probably just use 
`path::join("a", "b")`, as that will join them with the 
[`os::PATH_SEPARATOR`](https://github.com/apache/mesos/blob/edba292659e05d7a0989eb122f7824e1c21db8fb/3rdparty/stout/include/stout/os/constants.hpp#L26)
 which is `\` on Windows and `/` elsewhere.



3rdparty/stout/tests/path_tests.cpp
Lines 101-134 (patched)


Ditto. We can halve the amount of code by using `path::join`, and for cases 
where we have intermittent slashes etc., use `os::PATH_SEPARATOR` (I'd probably 
save that into a short local variable as it'll get referenced a lot in some of 
these.)



3rdparty/stout/tests/path_tests.cpp
Lines 172-187 (patched)


Ditto.



3rdparty/stout/tests/path_tests.cpp
Lines 212-235 (patched)


Now this is where it gets especially annoying, since we can't use 
`path::join` to test itself. But I would take:

```
EXPECT_EQ("\a\b\c\", path::join("\a\", "\b\", "\c\"));
```

and make it:

```
const string sep = stringify(os::PATH_SEPARATOR);
EXPECT_EQ(sep + "a" + sep + "b" + sep + "c" + sep, path::join(sep + "a" + 
sep, sep + "b" + sep, sep + "c" + sep));
```

At least this code can be reused, such that if the test gets changed, tests 
for both platforms are then updated always at the same time.


- Andrew Schwartzmeyer


On Oct. 24, 2017, 12:59 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63238/
> ---
> 
> (Updated Oct. 24, 2017, 12:59 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3442
> https://issues.apache.org/jira/browse/MESOS-3442
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> I checked #ifdefs in order to decide what tests to build and run on
> Window and Posix systems respectively.
> For Windows tests I mainly replaced the "slash" path separator with
> "backslash".
> I extracted common assertion before the #ifdef directive.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/path_tests.cpp 
> f8c14d5aefe0b49adb778da784143a328c96183d 
> 
> 
> Diff: https://reviews.apache.org/r/63238/diff/1/
> 
> 
> Testing
> ---
> 
> Modified Tests:
> TEST(PathTest, Basename)
> TEST(PathTest, Dirname)
> TEST(PathTest, Extension)
> TEST(PathTest, Join)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Re: Review Request 62877: Added RESOURCE_PROVIDER agent capability.

[Greg Mann]

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


Fix it, then Ship it!





include/mesos/mesos.proto
Lines 972-974 (patched)


Will this fit on two lines? Here and below.



include/mesos/mesos.proto
Lines 976 (patched)


s/provider/provide/

Here and below.


- Greg Mann


On Oct. 17, 2017, 11:24 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62877/
> ---
> 
> (Updated Oct. 17, 2017, 11:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8071
> https://issues.apache.org/jira/browse/MESOS-8071
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added RESOURCE_PROVIDER agent capability.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ba87339dbe341f4d16ceea74adc09647a3c07f32 
>   include/mesos/v1/mesos.proto a6d662fb26aa4f78ef20ffe6e013f7a45f7f8c21 
>   src/common/protobuf_utils.hpp ff0fd01b7a20f597ec6cd916f0bd9c41baa5fd51 
>   src/common/protobuf_utils.cpp 04f44f6f63e431c17ec67e234c8da58e7945294d 
> 
> 
> Diff: https://reviews.apache.org/r/62877/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63257: Added a few test helpers for creating disk resources.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 63094.

Failed command: `python.exe .\support\apply-reviews.py -n -r 63094`

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

Relevant logs:

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

```
error: patch failed: include/mesos/resource_provider/resource_provider.proto:52
error: include/mesos/resource_provider/resource_provider.proto: patch does not 
apply
error: patch failed: 
include/mesos/v1/resource_provider/resource_provider.proto:52
error: include/mesos/v1/resource_provider/resource_provider.proto: patch does 
not apply
error: patch failed: src/messages/messages.proto:637
error: src/messages/messages.proto: patch does not apply
error: patch failed: src/tests/resource_provider_manager_tests.cpp:286
error: src/tests/resource_provider_manager_tests.cpp: patch does not apply
error: patch failed: src/tests/resource_provider_validation_tests.cpp:98
error: src/tests/resource_provider_validation_tests.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Oct. 24, 2017, 6:06 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63257/
> ---
> 
> (Updated Oct. 24, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a few test helpers for creating disk resources.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 4b61f2dbd67a5e8a65a6655519c8f03e5d6c6954 
> 
> 
> Diff: https://reviews.apache.org/r/63257/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.

[Mesos Reviewbot Windows]

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



FAIL: The file 'C:DCOSmesosbuild-outputlogsapply-review-60620-stdout.log' 
already exists.

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

- Mesos Reviewbot Windows


On Oct. 24, 2017, 6:04 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63253/
> ---
> 
> (Updated Oct. 24, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017)
> for proper operation on Windows. Changed to use new URI methods
> introduced as part of MESOS-6705, and used powershell on Windows
> to replace "test" command on Linux.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 5078bd4d70698f5cbd14c971fcecfd58f8467a04 
> 
> 
> Diff: https://reviews.apache.org/r/63253/diff/1/
> 
> 
> Testing
> ---
> 
> Ran mesos-tests on both Windows and Linux with no errors.
> 
> Specifically ran mesos-tests with 
> `--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the 
> modified tests ran properly.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 63174: Added a benchmark for agent reregistration during master failover.

[Jiang Yan Xu]

> On Oct. 19, 2017, 6:38 p.m., Benjamin Mahler wrote:
> > Thanks Yan! I will dig in soon.
> > 
> > Just some quick questions:
> > 
> > (1) I thought during the meeting you said it was taking a minute, but 
> > looking at all the benchmark timings they're all under a second? Is it only 
> > the benchmark setup that's expensive here?
> > (2) Is this with the lock free event & run queues? If not, how much do they 
> > help?
> > (3) As an aside, it has come up before, but it would be useful to be able 
> > to force the messages to go through the remote stack rather than the local 
> > stack. No need to think about this yet, but just something to keep in mind 
> > as not being accurate in this benchmark.
> 
> Jiang Yan Xu wrote:
> 1) Yeah looks like it. I used to include the setup time so it was large. 
> 2) Yeah I have used `--enable-optimize --enable-lock-free-run-queue 
> --enable-lock-free-event-queue 
> --enable-last-in-first-out-fixed-size-semaphore`. I could compare with the 
> perf without them.
> 3) Right right I think we should keep that in mind and we should have 
> tests that cover the remote stack. For the case here I thought it would be a 
> simple and good-enough start since the local stack alright coveres the proto 
> (de)serliazation and the rest of the libprocess optimization that we recently 
> have improved.

Haha... actually the sub-second numbers in revision 1 were totally meaningless. 
I did `process::await(reregistered)` instead of 
`process::await(reregistered).await();` when I intended to wait for the 
results...

I did some optimization in rev 2 e.g., parallelize the message preparation, 
allocate from the stack instead of heap but I have to reduce the number of 
tasks to prevent it from running too long. 

PTAL.


- Jiang Yan


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


On Oct. 24, 2017, 11:05 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63174/
> ---
> 
> (Updated Oct. 24, 2017, 11:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Dmitry Zhuk, and Ilya Pronin.
> 
> 
> Bugs: MESOS-8098
> https://issues.apache.org/jira/browse/MESOS-8098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current benchmark is very simple: without framework involvement and 
> without agent retries but it's possible to add a number of others so I am 
> creating a new file for them.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
>   src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
>   src/tests/master_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63174/diff/2/
> 
> 
> Testing
> ---
> 
> Benchmark based off 
> https://github.com/apache/mesos/commit/41193181d6b75eeecae2729bf98007d9318e351a
>  (close to current HEAD).
> 
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 11.188008209secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (22404 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 20.868372615secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (37981 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 15.354579251secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (33766 ms)
> [--] 3 tests from 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (94151 ms total)
> 
> 
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 11.045441129secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (19959 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 

Re: Review Request 61946: Added validation of resource provider operations.

[Jie Yu]

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




src/master/validation.cpp
Lines 2266-2268 (patched)


This check is not necessary. Even if it passes the validation here, it 
might fail the following 'contains' check because some other operation in the 
same accept call used the resources. I'll simply remove this check here.


- Jie Yu


On Oct. 18, 2017, 1:51 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61946/
> ---
> 
> (Updated Oct. 18, 2017, 1:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7594
> https://issues.apache.org/jira/browse/MESOS-7594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation of resource provider operations.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585 
>   src/master/validation.cpp 01bc2e0ad4de0bad570453cdaafb260c61c511eb 
> 
> 
> Diff: https://reviews.apache.org/r/61946/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 63256: Added a master validation 'resource::validateSingleResourceProvider'.

[Jie Yu]

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

Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan 
Schlicht.


Repository: mesos


Description
---

This validates that all resources are from the same resource provider.
We don't allow an offer operation to be operating on resources from
different resource providers.


Diffs
-

  src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585 
  src/master/validation.cpp 42f5742386b59a983f7caaf3400c82b7ef4f8bbb 
  src/tests/master_validation_tests.cpp 
7da1be5233444aded64263d4a763fe2967459042 


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


Testing
---

make check


Thanks,

Jie Yu

Review Request 63257: Added a few test helpers for creating disk resources.

[Jie Yu]

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

Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan 
Schlicht.


Repository: mesos


Description
---

Added a few test helpers for creating disk resources.


Diffs
-

  src/tests/mesos.hpp 4b61f2dbd67a5e8a65a6655519c8f03e5d6c6954 


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


Testing
---

make check


Thanks,

Jie Yu

Review Request 63255: Added a helper to test if a resource has resource provider.

[Jie Yu]

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

Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan 
Schlicht.


Repository: mesos


Description
---

Added a helper to test if a resource has resource provider.


Diffs
-

  include/mesos/resources.hpp 9f959494f5c0376aa4ebd8f2571f4b1f45c83cb3 
  include/mesos/v1/resources.hpp a6216858136c82d5ebb5da3bb22c0b8a58a3b75d 
  src/common/resources.cpp 7ee4dae1389e037531aec533a3d235ee06443ea8 
  src/v1/resources.cpp 5c0a196cf87b377b345e53a36ed24727d52381ca 


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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 63174: Added a benchmark for agent reregistration during master failover.

[Jiang Yan Xu]

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

(Updated Oct. 24, 2017, 11:05 a.m.)


Review request for mesos, Benjamin Mahler, Dmitry Zhuk, and Ilya Pronin.


Changes
---

Refactor to put the message preparation work inside each TestSlave actor so 
they can be parallelized. Also fixed the bug that the test actually didn't wait 
for all the `SlaveReregisteredMessage`s...


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


Repository: mesos


Description
---

The current benchmark is very simple: without framework involvement and without 
agent retries but it's possible to add a number of others so I am creating a 
new file for them.


Diffs (updated)
-

  src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
  src/tests/CMakeLists.txt 386e0473c93d0a993248c7818067071d0c761c76 
  src/tests/master_benchmarks.cpp PRE-CREATION 


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

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


Testing (updated)
---

Benchmark based off 
https://github.com/apache/mesos/commit/41193181d6b75eeecae2729bf98007d9318e351a 
(close to current HEAD).

```
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
Starting reregistration for all agents
Reregistered 2000 agents with a total of 10 running tasks and 10 
completed tasks in 11.188008209secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
 (22404 ms)
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
Starting reregistration for all agents
Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
tasks in 20.868372615secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
 (37981 ms)
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
Starting reregistration for all agents
Reregistered 2 agents with a total of 10 running tasks and 0 completed 
tasks in 15.354579251secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
 (33766 ms)
[--] 3 tests from AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test 
(94151 ms total)


[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
Starting reregistration for all agents
Reregistered 2000 agents with a total of 10 running tasks and 10 
completed tasks in 11.045441129secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
 (19959 ms)
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
Starting reregistration for all agents
Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
tasks in 21.324309077secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
 (38490 ms)
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
Starting reregistration for all agents
Reregistered 2 agents with a total of 10 running tasks and 0 completed 
tasks in 14.68607521secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
 (32073 ms)
[--] 3 tests from AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test 
(90523 ms total)

```

Benchmark based off 
https://github.com/apache/mesos/commit/d9c90bf1d9c8b3a7dcc47be0cb773efff57cfb9d 
(before https://issues.apache.org/jira/browse/MESOS-7713 was merged)

```
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
Starting reregistration for all agents
Reregistered 2000 agents with a total of 10 running tasks and 10 
completed tasks in 23.217901878secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
 (38327 ms)
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
Starting reregistration for all agents
Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
tasks in 46.158610597secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
 (75280 ms)
[ RUN  ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
Starting reregistration for all agents
Reregistered 2 agents with a total of 10 running tasks and 0 completed 
tasks in 38.56781112secs
[   OK ] 
AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
 (68006 ms)
[--] 3 tests from AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test 
(181613 ms total)

[ RUN  ] 

Review Request 63253: Fixed DefaultExecutorTest for proper URI handling on Windows.

[Jeff Coffler]

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

Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li 
Li.


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


Repository: mesos


Description
---

Fixed changes from Gaston (git commit 772c8f5 on 10/02/2017)
for proper operation on Windows. Changed to use new URI methods
introduced as part of MESOS-6705, and used powershell on Windows
to replace "test" command on Linux.


Diffs
-

  src/tests/default_executor_tests.cpp 5078bd4d70698f5cbd14c971fcecfd58f8467a04 


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


Testing
---

Ran mesos-tests on both Windows and Linux with no errors.

Specifically ran mesos-tests with 
`--gtest_filter=MesosContainerizer/DefaultExecutorTest.*` to verify that the 
modified tests ran properly.


Thanks,

Jeff Coffler

Review Request 63254: Added a helper to test if a resource is a disk of a given type.

[Jie Yu]

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

Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and Jan 
Schlicht.


Repository: mesos


Description
---

Added a helper to test if a resource is a disk of a given type.


Diffs
-

  include/mesos/resources.hpp 9f959494f5c0376aa4ebd8f2571f4b1f45c83cb3 
  include/mesos/v1/resources.hpp a6216858136c82d5ebb5da3bb22c0b8a58a3b75d 
  src/common/resources.cpp 7ee4dae1389e037531aec533a3d235ee06443ea8 
  src/v1/resources.cpp 5c0a196cf87b377b345e53a36ed24727d52381ca 


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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 63223: Fixed whitespace problems in contribution guidelines.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Oct. 23, 2017, 7:30 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63223/
> ---
> 
> (Updated Oct. 23, 2017, 7:30 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Indentation needed to be consistently four spaces for sublists to render
> correctly, and blank linds needed to be removed for spacing to render
> correctly.
> 
> Also fixed outdated links to Getting Started for build instructions.
> 
> 
> Diffs
> -
> 
>   docs/advanced-contribution.md 5a89227bad3f2c9845420f90ae15bcee673fefd0 
>   docs/beginner-contribution.md d82079839faa760c42123aca52ac4d5dd8b2d64b 
> 
> 
> Diff: https://reviews.apache.org/r/63223/diff/1/
> 
> 
> Testing
> ---
> 
> This fixes the rendering problems at 
> https://mesos.apache.org/documentation/latest/advanced-contribution/
> 
> I tested by building the site locally and pendantically fixing until it 
> rendered correctly :P
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 63245: Ported and enabled Os Tests on Windows.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63245 was successfully built and tested.

Reviews applied: `['63245']`

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

- Mesos Reviewbot Windows


On Oct. 24, 2017, 11:30 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63245/
> ---
> 
> (Updated Oct. 24, 2017, 11:30 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3441
> https://issues.apache.org/jira/browse/MESOS-3441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> TEST_F(OsTest, Libraries)
> The test failed because no environmental variable was provided for
> Windows. I used "PATH" as environmental variable on Windows since
> there's no default variable where the linker should look into while
> linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on 
> Unix/Linux)
> 
> TEST_F(OsTest, Sleep)
> The test hanged because of the last assertion:
> ASSERT_ERROR(os::sleep(Milliseconds(-10)));
> Apparently, it didn't handle a negative sleep duration. By simply
> returning an error value when the duration is negative the problem was
> solved.
> 
> TEST_F(OsTest, SYMLINK_Size)
> First issue was this assertion:
> EXPECT_SOME_EQ(size, 
> os::stat::size(file,FollowSymlink::DO_NOT_FOLLOW_SYMLINK));
> It fails everytime because  is not a symbolic link. When calling
> os:stat:size with argument  set to "DO_NOT_FOLLOW_SYMLINK", it firstly
> checks whether the path it receives is a symlink in order to get the symlink 
> data
> for that path. If it's not a symlink, it returns an error.
> If you are supposed to be able to test the size of a non-link file in
> the context of not-following-a-link, the program should return the
> size of the non-link file instead of returning an error upon determining the
> nature of the path as not being a link file path. Right now, the assertion
> is not relevant since you'll get an error if you supply it a non-link file
> path rather thana symlink.
> 
> TEST_F(OsTest, SYMLINK_Realpath)
> The issue was that the symlink resolved to itself but it was tested
> against the path of the target file. Once the path of the target file
> was replaced by the path of the symlink, the test passed.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os.hpp 
> 1a81db61faa55d7043d75a012fe1e66b49bf601c 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 0db6d482ad19897db33d334bffe4b294da11a36f 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
>   3rdparty/stout/tests/os_tests.cpp 959b02d08f06481a64ef1a39acf23b062d5dc805 
> 
> 
> Diff: https://reviews.apache.org/r/63245/diff/1/
> 
> 
> Testing
> ---
> 
> Modified tests:
> TEST_F(OsTest, Libraries)
> TEST_F(OsTest, Sleep)
> TEST_F(OsTest, SYMLINK_Size)
> TEST_F(OsTest, SYMLINK_Realpath)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Re: Review Request 63238: Ported and enabled Path Tests on Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [63238]

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 Oct. 24, 2017, 10:59 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63238/
> ---
> 
> (Updated Oct. 24, 2017, 10:59 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3442
> https://issues.apache.org/jira/browse/MESOS-3442
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> I checked #ifdefs in order to decide what tests to build and run on
> Window and Posix systems respectively.
> For Windows tests I mainly replaced the "slash" path separator with
> "backslash".
> I extracted common assertion before the #ifdef directive.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/path_tests.cpp 
> f8c14d5aefe0b49adb778da784143a328c96183d 
> 
> 
> Diff: https://reviews.apache.org/r/63238/diff/1/
> 
> 
> Testing
> ---
> 
> Modified Tests:
> TEST(PathTest, Basename)
> TEST(PathTest, Dirname)
> TEST(PathTest, Extension)
> TEST(PathTest, Join)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Re: Review Request 63238: Ported and enabled Path Tests on Windows.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63238 was successfully built and tested.

Reviews applied: `['63238']`

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

- Mesos Reviewbot Windows


On Oct. 24, 2017, 7:59 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63238/
> ---
> 
> (Updated Oct. 24, 2017, 7:59 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3442
> https://issues.apache.org/jira/browse/MESOS-3442
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> I checked #ifdefs in order to decide what tests to build and run on
> Window and Posix systems respectively.
> For Windows tests I mainly replaced the "slash" path separator with
> "backslash".
> I extracted common assertion before the #ifdef directive.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/path_tests.cpp 
> f8c14d5aefe0b49adb778da784143a328c96183d 
> 
> 
> Diff: https://reviews.apache.org/r/63238/diff/1/
> 
> 
> Testing
> ---
> 
> Modified Tests:
> TEST(PathTest, Basename)
> TEST(PathTest, Dirname)
> TEST(PathTest, Extension)
> TEST(PathTest, Join)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Re: Review Request 63239: Ported and enabled Flag Tests on Windows.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 63239 was successfully built and tested.

Reviews applied: `['63239']`

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

- Mesos Reviewbot Windows


On Oct. 24, 2017, 7:57 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63239/
> ---
> 
> (Updated Oct. 24, 2017, 7:57 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3437
> https://issues.apache.org/jira/browse/MESOS-3437
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> They worked out of the box.
> Apparently the issues MESOS-5880 and MESOS-5937 which were causing the tests 
> to fail were solved.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/flags_tests.cpp 
> 88c8ee57c2bf328251513761498ef98d774be006 
> 
> 
> Diff: https://reviews.apache.org/r/63239/diff/1/
> 
> 
> Testing
> ---
> 
> Modified Tests:
> TEST(FlagsTest, LoadFromEnvironment)
> TEST(FlagsTest, DuplicatesFromEnvironment)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Re: Review Request 63245: Ported and enabled Os Tests on Windows.

[Raluca Miclea]

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

(Updated Oct. 24, 2017, 9:30 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description (updated)
---

TEST_F(OsTest, Libraries)
The test failed because no environmental variable was provided for
Windows. I used "PATH" as environmental variable on Windows since
there's no default variable where the linker should look into while
linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on Unix/Linux)

TEST_F(OsTest, Sleep)
The test hanged because of the last assertion:
ASSERT_ERROR(os::sleep(Milliseconds(-10)));
Apparently, it didn't handle a negative sleep duration. By simply
returning an error value when the duration is negative the problem was
solved.

TEST_F(OsTest, SYMLINK_Size)
First issue was this assertion:
EXPECT_SOME_EQ(size, os::stat::size(file,FollowSymlink::DO_NOT_FOLLOW_SYMLINK));
It fails everytime because  is not a symbolic link. When calling
os:stat:size with argument  set to "DO_NOT_FOLLOW_SYMLINK", it firstly
checks whether the path it receives is a symlink in order to get the symlink 
data
for that path. If it's not a symlink, it returns an error.
If you are supposed to be able to test the size of a non-link file in
the context of not-following-a-link, the program should return the
size of the non-link file instead of returning an error upon determining the
nature of the path as not being a link file path. Right now, the assertion
is not relevant since you'll get an error if you supply it a non-link file
path rather thana symlink.

TEST_F(OsTest, SYMLINK_Realpath)
The issue was that the symlink resolved to itself but it was tested
against the path of the target file. Once the path of the target file
was replaced by the path of the symlink, the test passed.


Diffs
-

  3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c 
  3rdparty/stout/include/stout/os/windows/stat.hpp 
0db6d482ad19897db33d334bffe4b294da11a36f 
  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
  3rdparty/stout/tests/os_tests.cpp 959b02d08f06481a64ef1a39acf23b062d5dc805 


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


Testing
---

Modified tests:
TEST_F(OsTest, Libraries)
TEST_F(OsTest, Sleep)
TEST_F(OsTest, SYMLINK_Size)
TEST_F(OsTest, SYMLINK_Realpath)


Thanks,

Raluca Miclea

Re: Review Request 63245: Ported and enabled Os Tests on Windows.

[Raluca Miclea]

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

(Updated Oct. 24, 2017, 9:28 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

TEST_F(OsTest, Libraries)
The test failed because no environmental variable was provided for
Windows. I used "PATH" as environmental variable on Windows since
there's no default variable where the linker should look into while
linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on Unix/Linux)

TEST_F(OsTest, Sleep)
The test hanged because of the last assertion:
ASSERT_ERROR(os::sleep(Milliseconds(-10)));
Apparently, it didn't handle a negative sleep duration. By simply
returning an error value when the duration is negative the problem was
solved.

TEST_F(OsTest, SYMLINK_Size)
First issue was this assertion:
EXPECT_SOME_EQ(size, os::stat::size(file,FollowSymlink::DO_NOT_FOLLOW_SYMLINK));
It fails everytime because  is not a symbolic link. When calling
os:stat:size with argument  set to "DO_NOT_FOLLOW_SYMLINK", it firstly
checks whether the path it receives is a symlink in order to get the symlink 
data
for that path. If it's not a symlink, it returns an error.
If you are supposed to be able to test the size of a non-link file in
the context of not-following-a-link, the program should return the
size of the non-link file instead of returning an error upon determining the
nature of the path as not being a link file path. Right now, the assertion
is not relevant since you'll get an error if you supply it a non-link file
path rather thana symlink.

TEST_F_TEMP_DISABLED_ON_WINDOWS(OsTest, SYMLINK_Realpath)
The issue was that the symlink resolved to itself but it was tested
against the path of the target file. Once the path of the target file
was replaced by the path of the symlink, the test passed.


Diffs
-

  3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c 
  3rdparty/stout/include/stout/os/windows/stat.hpp 
0db6d482ad19897db33d334bffe4b294da11a36f 
  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
  3rdparty/stout/tests/os_tests.cpp 959b02d08f06481a64ef1a39acf23b062d5dc805 


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


Testing (updated)
---

Modified tests:
TEST_F(OsTest, Libraries)
TEST_F(OsTest, Sleep)
TEST_F(OsTest, SYMLINK_Size)
TEST_F(OsTest, SYMLINK_Realpath)


Thanks,

Raluca Miclea

Review Request 63245: Ported and enabled Os Tests on Windows.

[Raluca Miclea]

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

Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

TEST_F(OsTest, Libraries)
The test failed because no environmental variable was provided for
Windows. I used "PATH" as environmental variable on Windows since
there's no default variable where the linker should look into while
linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on Unix/Linux)

TEST_F(OsTest, Sleep)
The test hanged because of the last assertion:
ASSERT_ERROR(os::sleep(Milliseconds(-10)));
Apparently, it didn't handle a negative sleep duration. By simply
returning an error value when the duration is negative the problem was
solved.

TEST_F(OsTest, SYMLINK_Size)
First issue was this assertion:
EXPECT_SOME_EQ(size, os::stat::size(file,FollowSymlink::DO_NOT_FOLLOW_SYMLINK));
It fails everytime because  is not a symbolic link. When calling
os:stat:size with argument  set to "DO_NOT_FOLLOW_SYMLINK", it firstly
checks whether the path it receives is a symlink in order to get the symlink 
data
for that path. If it's not a symlink, it returns an error.
If you are supposed to be able to test the size of a non-link file in
the context of not-following-a-link, the program should return the
size of the non-link file instead of returning an error upon determining the
nature of the path as not being a link file path. Right now, the assertion
is not relevant since you'll get an error if you supply it a non-link file
path rather thana symlink.

TEST_F_TEMP_DISABLED_ON_WINDOWS(OsTest, SYMLINK_Realpath)
The issue was that the symlink resolved to itself but it was tested
against the path of the target file. Once the path of the target file
was replaced by the path of the symlink, the test passed.


Diffs
-

  3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c 
  3rdparty/stout/include/stout/os/windows/stat.hpp 
0db6d482ad19897db33d334bffe4b294da11a36f 
  3rdparty/stout/include/stout/windows/os.hpp 
09ddec6d69472cd13b453fe1a77fdbe343fc23c8 
  3rdparty/stout/tests/os_tests.cpp 959b02d08f06481a64ef1a39acf23b062d5dc805 


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


Testing
---

Modified tests:
TEST_F(OsTest, Libraries)
TEST_F(OsTest, Sleep)
TEST_F(OsTest, SYMLINK_Size)
TEST_F_TEMP_DISABLED_ON_WINDOWS(OsTest, SYMLINK_Realpath)


Thanks,

Raluca Miclea

Re: Review Request 63239: Ported and enabled Flag Tests on Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [63239]

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 Oct. 24, 2017, 7:57 a.m., Raluca Miclea wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63239/
> ---
> 
> (Updated Oct. 24, 2017, 7:57 a.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-3437
> https://issues.apache.org/jira/browse/MESOS-3437
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> They worked out of the box.
> Apparently the issues MESOS-5880 and MESOS-5937 which were causing the tests 
> to fail were solved.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/flags_tests.cpp 
> 88c8ee57c2bf328251513761498ef98d774be006 
> 
> 
> Diff: https://reviews.apache.org/r/63239/diff/1/
> 
> 
> Testing
> ---
> 
> Modified Tests:
> TEST(FlagsTest, LoadFromEnvironment)
> TEST(FlagsTest, DuplicatesFromEnvironment)
> 
> 
> Thanks,
> 
> Raluca Miclea
> 
>

Re: Review Request 63238: Ported and enabled Path Tests on Windows.

[Raluca Miclea]

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

(Updated Oct. 24, 2017, 7:59 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

I checked #ifdefs in order to decide what tests to build and run on
Window and Posix systems respectively.
For Windows tests I mainly replaced the "slash" path separator with
"backslash".
I extracted common assertion before the #ifdef directive.


Diffs
-

  3rdparty/stout/tests/path_tests.cpp f8c14d5aefe0b49adb778da784143a328c96183d 


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


Testing (updated)
---

Modified Tests:
TEST(PathTest, Basename)
TEST(PathTest, Dirname)
TEST(PathTest, Extension)
TEST(PathTest, Join)


Thanks,

Raluca Miclea

Re: Review Request 63239: Ported and enabled Flag Tests on Windows.

[Raluca Miclea]

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

(Updated Oct. 24, 2017, 7:57 a.m.)


Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

They worked out of the box.
Apparently the issues MESOS-5880 and MESOS-5937 which were causing the tests to 
fail were solved.


Diffs
-

  3rdparty/stout/tests/flags_tests.cpp 88c8ee57c2bf328251513761498ef98d774be006 


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


Testing (updated)
---

Modified Tests:
TEST(FlagsTest, LoadFromEnvironment)
TEST(FlagsTest, DuplicatesFromEnvironment)


Thanks,

Raluca Miclea

Review Request 63239: Ported and enabled Flag Tests on Windows.

[Raluca Miclea]

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

Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

They worked out of the box.
Apparently the issues MESOS-5880 and MESOS-5937 which were causing the tests to 
fail were solved.


Diffs
-

  3rdparty/stout/tests/flags_tests.cpp 88c8ee57c2bf328251513761498ef98d774be006 


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


Testing
---


Thanks,

Raluca Miclea

Review Request 63238: Ported and enabled Path Tests on Windows.

[Raluca Miclea]

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

Review request for mesos and Andrew Schwartzmeyer.


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


Repository: mesos


Description
---

I checked #ifdefs in order to decide what tests to build and run on
Window and Posix systems respectively.
For Windows tests I mainly replaced the "slash" path separator with
"backslash".
I extracted common assertion before the #ifdef directive.


Diffs
-

  3rdparty/stout/tests/path_tests.cpp f8c14d5aefe0b49adb778da784143a328c96183d 


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


Testing
---


Thanks,

Raluca Miclea

Re: Review Request 63232: Converted SlaveInfo resources from the registry during master recovery.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [63232]

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 Oct. 23, 2017, 11:56 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63232/
> ---
> 
> (Updated Oct. 23, 2017, 11:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Greg Mann.
> 
> 
> Bugs: MESOS-7851
> https://issues.apache.org/jira/browse/MESOS-7851
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The intention is for us to work with resources in the 
> `post-reservation-refinement` format
> in the master memory, and store them in the master registry or agent 
> checkpoint in
> the `pre` format in order to support downgrades. In this case, we correctly 
> store the `pre`
> format resources in the master registry, but we don't convert them back when 
> we read it out
> of the registry. This patch converts the resources back to `post` when we 
> read from the registry.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 4b76648b4887724e166ff95e002ee26c8bfef006 
> 
> 
> Diff: https://reviews.apache.org/r/63232/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>