Review Request 49906: Add lawrencew to contributors

[Lawrence Wu]

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

Review request for mesos and Ian Downes.


Repository: mesos


Description
---

Add lawrencew to contributors


Diffs
-

  docs/contributors.yaml 7272b59bfff1c30958f705b849b75f5cc656321e 

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


Testing
---


Thanks,

Lawrence Wu

Re: Review Request 49903: Printed empty set if `Resources` instance is empty.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49903]

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

- Mesos ReviewBot


On July 11, 2016, 4:31 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49903/
> ---
> 
> (Updated July 11, 2016, 4:31 p.m.)
> 
> 
> Review request for mesos, Isabel Jimenez, Michael Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, empty `Resources` instances have been omitted
> if printed to a stream. This showed up as double space, which is
> confusing. Now, an empty `Resources` instance is explicitly printed.
> 
> Prior:
> <...> with oversubscribed resources  (total: <...>, allocated: )
> 
> After:
> <...> with oversubscribed resources {} (total: <...>, allocated: {})
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/49903/diff/
> 
> 
> Testing
> ---
> 
> make check + visual inspection
> 
> Prior:
> ```
> Agent 0aa85f40-9409-45d7-b45c-6b6905b2836d-S0 (192.168.0.18) updated with 
> oversubscribed resources  (total: cpus(*):8; mem(*):15360; disk(*):470847; 
> ports(*):[31000-32000], allocated: )
> ```
> 
> After:
> ```
> Agent cd200600-03f7-4f5a-8721-94659c149145-S0 (192.168.0.18) updated with 
> oversubscribed resources {} (total: cpus(*):8; mem(*):15360; disk(*):470847; 
> ports(*):[31000-32000], allocated: {})
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 49841: Reorganized the routing tests into basic and advanced groups.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On July 8, 2016, 11:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49841/
> ---
> 
> (Updated July 8, 2016, 11:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Advanced tests need some higher version of libnl.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

[Benjamin Bannier]

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




src/launcher/executor.cpp (line 90)


I would much prefer this to not force each and every caller to deal with 
different function signatures.

We should probably try to find a way to use identical signatures on every 
platform, and then possible make the underlying implementation do what is 
actually supported.



src/launcher/executor.cpp (line 376)


It seems we should be able to do away with conditional compilation here if 
we'd push the check for platform support into the capabilities implementation.



src/launcher/executor.cpp (line 397)


Same here, we should try to remove the conditional compilation and instead 
push the checks for platform support into the capabilities infrastructure.



src/slave/containerizer/mesos/containerizer.cpp (line 270)


This could again be simplified by pushing the check for platform support 
into the capabilities infrastructure.



src/slave/containerizer/mesos/containerizer.cpp (line 271)


I think moving the capabilities check out of the loop would improve 
readibilty of this loop.



src/slave/containerizer/mesos/containerizer.cpp (line 1299)


Remove conditional compilation once member exists for all platforms.



src/slave/containerizer/mesos/launch.cpp (line 31)


Sort alphabetically.



src/slave/containerizer/mesos/launch.cpp (line 49)


As currently written this needs to be compiled conditionally as 
`Capabilities` is not defined on non-Linux platforms.



src/slave/containerizer/mesos/launch.cpp (line 280)


Why do we need to require a shell command here?



src/slave/containerizer/mesos/launch.cpp (line 281)


The nesting of these preprocessor checks (where one checks a negative and 
the other for a positive) on top of a already deeply control flow makes this 
really hard to read.

I believe this could again be simplified by pushing the check for platform 
support into the capabilities infrastructure.


- Benjamin Bannier


On May 26, 2016, 5:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46798/
> ---
> 
> (Updated May 26, 2016, 5:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces linux capability based security for unified
> containerizer. A new agent flag \`allowed_capabilities\` has been
> introduced to override the default capabilities of the user or the
> capabilities requested by the user.
> 
> This feature is only available on linux.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
>   src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a1a00020668f6da8d611f26e5637afffc87d09ba 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   src/slave/containerizer/mesos/launch.hpp 
> c716e0396736d1f2f60ec31540f12f4f7597d081 
>   src/slave/containerizer/mesos/launch.cpp 
> e22106b014c871e2184a15c2ab154a0674874e47 
>   src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
>   src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
>   src/tests/container_logger_tests.cpp 
> efadceafca5721bce4dbffadb35f54fd5365abb0 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 4293416ac8434e9eb7e80724480a54936a2fe24a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 09742ff21513dc2570684d384b257868dd57a9ce 
> 
> Diff: https://reviews.apache.org/r/46798/diff/
> 
> 
> Testing
> ---
> 
> make check; used mesos cli to test end to end functionality.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 49841: Reorganized the routing tests into basic and advanced groups.

[Jie Yu]

> On July 11, 2016, 2:46 p.m., Qian Zhang wrote:
> > So even we reorganize the routing tests into basic and advanced groups, all 
> > these tests are still in the same file (routing_tests.cpp which is 
> > currently guarded by `WITH_NETWORK_ISOLATOR` in Makefile.am) which means 
> > they will be compiled as a whole. So what if user just wants to run basic 
> > tests but not the advanded tests? In this case, they also need the newer 
> > version of libnl? Maybe we should reorganize them into two separate files, 
> > e.g., routing_tests.cpp and advanced_routing_tests.cpp, the former can work 
> > with older version of libnl, and the later has to work with newer version 
> > of libnl.

I can use #ifdef blocks to solve that for now.


- Jie


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


On July 8, 2016, 11:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49841/
> ---
> 
> (Updated July 8, 2016, 11:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Advanced tests need some higher version of libnl.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49841: Reorganized the routing tests into basic and advanced groups.

[Jie Yu]

> On July 11, 2016, 3:42 p.m., Avinash sridharan wrote:
> > src/tests/containerizer/routing_tests.cpp, line 258
> > 
> >
> > Instead of failing over here should we be disabling these tests if the 
> > libnl version is not correct?
> > 
> > That's the convention we have using with the other tests 
> > (cgroups/netcls). Does it make sense to add a filter for these tests?

Yeah, we should follow up with a patch. This patch just do the splitting.


- Jie


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


On July 8, 2016, 11:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49841/
> ---
> 
> (Updated July 8, 2016, 11:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Advanced tests need some higher version of libnl.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49883: Renamed and lambda-ified a function in allocator tests.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On July 11, 2016, 9:41 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49883/
> ---
> 
> (Updated July 11, 2016, 9:41 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed and lambda-ified a function in allocator tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49883/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49839: Removed the network isolator specific check in netlink socket method.

[Jie Yu]

> On July 11, 2016, 2:10 p.m., Qian Zhang wrote:
> > So the main purposal of this patch is to remove the dependency (e.g., 
> > `nl_has_capability()`) of higher version of libnl from `routing::socket()` 
> > which can then work with lower version of libnl, right?

Yeah.


- Jie


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


On July 8, 2016, 11:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49839/
> ---
> 
> (Updated July 8, 2016, 11:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This check has been performed in the port mapping isolator as well in
> the corresponding tests.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/internal.hpp 939634637d13497e77f4a8c3d4257eb98c92fa3f 
> 
> Diff: https://reviews.apache.org/r/49839/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49880: Fixed typos in comments in allocator tests.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On July 11, 2016, 8:48 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49880/
> ---
> 
> (Updated July 11, 2016, 8:48 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typos in comments in allocator tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
>   src/tests/master_allocator_tests.cpp 
> 4beebff9da791c9c80b0bfbe5d0b6f19098f7789 
> 
> Diff: https://reviews.apache.org/r/49880/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 49903: Printed empty set if `Resources` instance is empty.

[Alexander Rukletsov]

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

Review request for mesos, Isabel Jimenez, Michael Park, and Till Toenshoff.


Repository: mesos


Description
---

Prior to this patch, empty `Resources` instances have been omitted
if printed to a stream. This showed up as double space, which is
confusing. Now, an empty `Resources` instance is explicitly printed.

Prior:
<...> with oversubscribed resources  (total: <...>, allocated: )

After:
<...> with oversubscribed resources {} (total: <...>, allocated: {})


Diffs
-

  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing
---

make check + visual inspection

Prior:
```
Agent 0aa85f40-9409-45d7-b45c-6b6905b2836d-S0 (192.168.0.18) updated with 
oversubscribed resources  (total: cpus(*):8; mem(*):15360; disk(*):470847; 
ports(*):[31000-32000], allocated: )
```

After:
```
Agent cd200600-03f7-4f5a-8721-94659c149145-S0 (192.168.0.18) updated with 
oversubscribed resources {} (total: cpus(*):8; mem(*):15360; disk(*):470847; 
ports(*):[31000-32000], allocated: {})
```


Thanks,

Alexander Rukletsov

Re: Review Request 48313: Creation and deletion of persistent volumes across agent restart.

[Neil Conway]

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


Fix it, then Ship it!





src/slave/slave.cpp (line 2506)


"the operations"



src/slave/slave.cpp (line 2507)


"the update to checkpoints" seems vague -- how about "the update to 
checkpointed resources"?



src/slave/slave.cpp (line 2521)


"committing"



src/slave/slave.cpp (line 4736)


This comment and the comment a few lines below ("Attempt to sync...") seem 
a bit redundant. Can we consolidate them into a single comment?



src/slave/slave.cpp (line 4762)


Should we also remove the target resources in the case when target and 
checkpointed resources are the same?



src/slave/slave.cpp (line 4767)


Can we include the error message from `rm` in the log message we emit?



src/slave/state.cpp (line 704)


Needs a newline after it, per style guide. ("Inside a code block, every 
multi-line statement should be followed by one empty line.")



src/slave/state.cpp (line 712)


Can we avoid reusing the same variable name? How about `infoPath` and 
`targetPath`?



src/slave/state.cpp (line 714)


Does this merit a `LOG(INFO)`? Seems like the common case is that a target 
checkpointed resource file will not be found.



src/slave/state.cpp (line 720)


Needs a newline.



src/slave/state.cpp (line 732)


Should be `string`, not `std::string`.


Can you update the review summary to describe the new protocol implemented by 
this RR?

- Neil Conway


On July 1, 2016, 9:39 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48313/
> ---
> 
> (Updated July 1, 2016, 9:39 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the agent receives CheckpointedResourcesMessage, we store the
> target checkpoint on disk. On successful create and destroy of
> persistent volumes as a part of handling this messages, we commit
> the checkpoint on the disk, and clear the target checkpoint.
> 
> However, incase of any failure we do not commit the checkpoint to
> disk, and exit the agent. When the agent restarts and there is a
> target checkpoint present on disk which differs from the committed
> checkpoint, we retry to sync the target and committed checkpoint.
> On success, we reregister the agent with the master, but in case it
> fails, we do not commit the checkpoint and the agent exists.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp 339e539863c678b6ed4d4670d75c7ff4c54daa79 
>   src/slave/paths.cpp 03157f93b1e703006f95ef6d0a30afae375dcdb5 
>   src/slave/slave.hpp 484ba758b4c87935aabd2f76a0e654a3c6d09167 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
>   src/slave/state.hpp 0de2a4ee4fabaad612c4526166157b001c380bdb 
>   src/slave/state.cpp 9cec0868b1187ed3ccac7f065e8a21c2f52178d9 
> 
> Diff: https://reviews.apache.org/r/48313/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49874: Added logrotate_container_logger for running mesos tests.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On July 11, 2016, 3:48 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49874/
> ---
> 
> (Updated July 11, 2016, 3:48 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added logrotate_container_logger for running mesos tests.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt d31440cb5e784d3e4f1236ac45001e80384f36f6 
>   src/slave/cmake/SlaveConfigure.cmake 
> ca4575653e00e8f868160976344ac1d57b5f7d27 
> 
> Diff: https://reviews.apache.org/r/49874/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49840: Fixed the right angle brackets in routing tests.

[Benjamin Bannier]

> On July 11, 2016, 4:15 p.m., Qian Zhang wrote:
> > Is this (">>") the coding convention of Mesos? If so, we may need to 
> > mention it in 
> > https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md. 
> > And I see there are still a lot of code in Mesos using "> >" now, maybe we 
> > need a separate patch to update all of them?

The `> >` form is the exception to how C++ is used nowadays; that form works 
around a deficiency where these right angle brackets might have been understood 
as some form of `operator>>`. In C++11 which is used in Mesos we do not need 
this workaround. We usually fix these as we touch code, but I filed MESOS-5830 
for the sweep.


- Benjamin


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


On July 9, 2016, 1:57 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49840/
> ---
> 
> (Updated July 9, 2016, 1:57 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the right angle brackets in routing tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49840/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 49874: Added logrotate_container_logger for running mesos tests.

[Srinivas Brahmaroutu]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added logrotate_container_logger for running mesos tests.


Diffs
-

  src/slave/CMakeLists.txt d31440cb5e784d3e4f1236ac45001e80384f36f6 
  src/slave/cmake/SlaveConfigure.cmake ca4575653e00e8f868160976344ac1d57b5f7d27 

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


Testing
---


Thanks,

Srinivas Brahmaroutu

Review Request 49870: Added test executables required to run tests.

[Srinivas Brahmaroutu]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added test executables required to run tests.


Diffs
-

  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49863: Added Test Modules that are loaded by mesos tests.

[Srinivas Brahmaroutu]

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

(Updated July 11, 2016, 3:47 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added Test Modules that are loaded by mesos tests.


Diffs (updated)
-

  CMakeLists.txt 31601a2280fa4a07df53e4e332a7e2fb0199079c 
  cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
  src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

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


Testing
---

Make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49862: Changed libmesos from static library to a shared library.

[Srinivas Brahmaroutu]

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

(Updated July 11, 2016, 3:47 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Changed libmesos from static library to a shared library.


Diffs (updated)
-

  3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
eeb27860f6f95d297ccfe273ed76de5355b50ff8 
  3rdparty/http-parser/CMakeLists.txt.template 
7f7105f5d55fd66f3e826c1c37c0074775bf89ea 
  src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
  src/master/cmake/MasterConfigure.cmake 
6bbd7e87273976f40527d719cc9450ff9a1d2ac7 
  src/slave/cmake/SlaveConfigure.cmake ca4575653e00e8f868160976344ac1d57b5f7d27 

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


Testing (updated)
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49688: Added cmake build for mesos tests.

[Srinivas Brahmaroutu]

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

(Updated July 11, 2016, 3:46 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added cmake build for mesos tests.


Diffs (updated)
-

  cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
  src/tests/cmake/TestsConfigure.cmake PRE-CREATION 

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


Testing
---

cmake ..
cmake check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49841: Reorganized the routing tests into basic and advanced groups.

[Avinash sridharan]

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




src/tests/containerizer/routing_tests.cpp (line 207)


Instead of failing over here should we be disabling these tests if the 
libnl version is not correct?

That's the convention we have using with the other tests (cgroups/netcls). 
Does it make sense to add a filter for these tests?


- Avinash sridharan


On July 8, 2016, 11:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49841/
> ---
> 
> (Updated July 8, 2016, 11:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Advanced tests need some higher version of libnl.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49839: Removed the network isolator specific check in netlink socket method.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On July 8, 2016, 11:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49839/
> ---
> 
> (Updated July 8, 2016, 11:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This check has been performed in the port mapping isolator as well in
> the corresponding tests.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/internal.hpp 939634637d13497e77f4a8c3d4257eb98c92fa3f 
> 
> Diff: https://reviews.apache.org/r/49839/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49840: Fixed the right angle brackets in routing tests.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On July 8, 2016, 11:57 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49840/
> ---
> 
> (Updated July 8, 2016, 11:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the right angle brackets in routing tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49840/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49892: Removed unnecessary await from test.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On July 11, 2016, 2:04 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49892/
> ---
> 
> (Updated July 11, 2016, 2:04 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary await from test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> e43b264b9f67d4cd965aee143cc42a1034ac9952 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 69ec707d4f7c16743a756ad14005cc84a8cdcc54 
> 
> Diff: https://reviews.apache.org/r/49892/diff/
> 
> 
> Testing
> ---
> 
> make check --gtest_repeat=100
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49835: Moved netlink cleanup functions to separate files.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On July 9, 2016, 6:44 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49835/
> ---
> 
> (Updated July 9, 2016, 6:44 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another effort to allow linking with a lower version of libnl.
> This patch moved cleanup functions into their specific files.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/diagnosis/diagnosis.hpp 
> 447f342c36e375159f5e48faf16d45aa85bddc52 
>   src/linux/routing/diagnosis/diagnosis.cpp 
> ce546978eed062affe7986eec7dfcd0f26a0a055 
>   src/linux/routing/filter/internal.hpp 
> 6c174e1c18970f2742e8ac147772267c67b2283a 
>   src/linux/routing/internal.hpp 939634637d13497e77f4a8c3d4257eb98c92fa3f 
>   src/linux/routing/link/internal.hpp 
> eb89b9a87db3f92df033e6dd1d586345370b8697 
>   src/linux/routing/queueing/internal.hpp 
> 151d1b1d01b31af55a888b659df5370838c7cabc 
> 
> Diff: https://reviews.apache.org/r/49835/diff/
> 
> 
> Testing
> ---
> 
> make check on fedora 23
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49835: Moved netlink cleanup functions to separate files.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On July 8, 2016, 10:44 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49835/
> ---
> 
> (Updated July 8, 2016, 10:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another effort to allow linking with a lower version of libnl.
> This patch moved cleanup functions into their specific files.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/diagnosis/diagnosis.hpp 
> 447f342c36e375159f5e48faf16d45aa85bddc52 
>   src/linux/routing/diagnosis/diagnosis.cpp 
> ce546978eed062affe7986eec7dfcd0f26a0a055 
>   src/linux/routing/filter/internal.hpp 
> 6c174e1c18970f2742e8ac147772267c67b2283a 
>   src/linux/routing/internal.hpp 939634637d13497e77f4a8c3d4257eb98c92fa3f 
>   src/linux/routing/link/internal.hpp 
> eb89b9a87db3f92df033e6dd1d586345370b8697 
>   src/linux/routing/queueing/internal.hpp 
> 151d1b1d01b31af55a888b659df5370838c7cabc 
> 
> Diff: https://reviews.apache.org/r/49835/diff/
> 
> 
> Testing
> ---
> 
> make check on fedora 23
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49892: Removed unnecessary await from test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49892]

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

- Mesos ReviewBot


On July 11, 2016, 2:04 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49892/
> ---
> 
> (Updated July 11, 2016, 2:04 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary await from test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> e43b264b9f67d4cd965aee143cc42a1034ac9952 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 69ec707d4f7c16743a756ad14005cc84a8cdcc54 
> 
> Diff: https://reviews.apache.org/r/49892/diff/
> 
> 
> Testing
> ---
> 
> make check --gtest_repeat=100
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49841: Reorganized the routing tests into basic and advanced groups.

[Qian Zhang]

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



So even we reorganize the routing tests into basic and advanced groups, all 
these tests are still in the same file (routing_tests.cpp which is currently 
guarded by `WITH_NETWORK_ISOLATOR` in Makefile.am) which means they will be 
compiled as a whole. So what if user just wants to run basic tests but not the 
advanded tests? In this case, they also need the newer version of libnl? Maybe 
we should reorganize them into two separate files, e.g., routing_tests.cpp and 
advanced_routing_tests.cpp, the former can work with older version of libnl, 
and the later has to work with newer version of libnl.

- Qian Zhang


On July 9, 2016, 7:57 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49841/
> ---
> 
> (Updated July 9, 2016, 7:57 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Advanced tests need some higher version of libnl.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49840: Fixed the right angle brackets in routing tests.

[Qian Zhang]

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


Ship it!




Is this (">>") the coding convention of Mesos? If so, we may need to mention it 
in https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md. And 
I see there are still a lot of code in Mesos using "> >" now, maybe we need a 
separate patch to update all of them?

- Qian Zhang


On July 9, 2016, 7:57 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49840/
> ---
> 
> (Updated July 9, 2016, 7:57 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the right angle brackets in routing tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49840/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49839: Removed the network isolator specific check in netlink socket method.

[Qian Zhang]

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


Ship it!




So the main purposal of this patch is to remove the dependency (e.g., 
`nl_has_capability()`) of higher version of libnl from `routing::socket()` 
which can then work with lower version of libnl, right?

- Qian Zhang


On July 9, 2016, 7:57 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49839/
> ---
> 
> (Updated July 9, 2016, 7:57 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This check has been performed in the port mapping isolator as well in
> the corresponding tests.
> 
> 
> Diffs
> -
> 
>   src/linux/routing/internal.hpp 939634637d13497e77f4a8c3d4257eb98c92fa3f 
> 
> Diff: https://reviews.apache.org/r/49839/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49892: Removed unnecessary await from test.

[Joerg Schad]

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

(Updated July 11, 2016, 2:04 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Removed unnecessary await from test.


Diffs (updated)
-

  src/tests/master_authorization_tests.cpp 
e43b264b9f67d4cd965aee143cc42a1034ac9952 
  src/tests/master_slave_reconciliation_tests.cpp 
69ec707d4f7c16743a756ad14005cc84a8cdcc54 

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


Testing
---

make check --gtest_repeat=100


Thanks,

Joerg Schad

Re: Review Request 49892: Removed unnecessary await from test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49892]

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

- Mesos ReviewBot


On July 11, 2016, 12:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49892/
> ---
> 
> (Updated July 11, 2016, 12:25 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary await from test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> e43b264b9f67d4cd965aee143cc42a1034ac9952 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 69ec707d4f7c16743a756ad14005cc84a8cdcc54 
> 
> Diff: https://reviews.apache.org/r/49892/diff/
> 
> 
> Testing
> ---
> 
> make check --gtest_repeat=100
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 32079: added a few more presentations

[Joerg Schad]

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



I really like the idea of updating the list with new presentations. Could we 
just update this list to a more recent point in time? Over the past year there 
have been many updated/new presentations to the topics covered here...

- Joerg Schad


On March 15, 2015, 2:08 a.m., Nancy  Ko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32079/
> ---
> 
> (Updated March 15, 2015, 2:08 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2444
> https://issues.apache.org/jira/browse/MESOS-2444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> continuation of  https://reviews.apache.org/r/31790/ - added more 
> presentations
> 
> 
> Diffs
> -
> 
>   docs/mesos-presentations.md 4ae66f345970b2683d28307d1bed8333668d672d 
> 
> Diff: https://reviews.apache.org/r/32079/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Nancy  Ko
> 
>

Re: Review Request 49892: Removed unnecessary await from test.

[Neil Conway]

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




src/tests/master_slave_reconciliation_tests.cpp (line 600)


Should we check that the status update is for the `TASK_RUNNING` state?


- Neil Conway


On July 11, 2016, 12:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49892/
> ---
> 
> (Updated July 11, 2016, 12:25 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary await from test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> e43b264b9f67d4cd965aee143cc42a1034ac9952 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 69ec707d4f7c16743a756ad14005cc84a8cdcc54 
> 
> Diff: https://reviews.apache.org/r/49892/diff/
> 
> 
> Testing
> ---
> 
> make check --gtest_repeat=100
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49692: Removed an unnecessary `Clock::settle` from a test case.

[Joerg Schad]

> On July 8, 2016, 5:23 p.m., Vinod Kone wrote:
> > src/tests/master_slave_reconciliation_tests.cpp, line 620
> > 
> >
> > not yours.
> > 
> > @joerg: can you remind me why you do AWAIT_READY(status) here instead 
> > of up at #602? also why do you need AWAIT_READY(_statusUpdate)?

https://reviews.apache.org/r/49892/


- Joerg


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


On July 6, 2016, 8:38 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49692/
> ---
> 
> (Updated July 6, 2016, 8:38 a.m.)
> 
> 
> Review request for mesos, Joerg Schad and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unnecessary `Clock::settle` from a test case.
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 87cff8cbce0af5cfaf369854b812599d36c762d9 
> 
> Diff: https://reviews.apache.org/r/49692/diff/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests 
> --gtest_filter="MasterSlaveReconciliationTest.SlaveReregisterActiveFrameworks"
>  --gtest_repeat=700 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 49892: Removed unnecessary await from test.

[Joerg Schad]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Removed unnecessary await from test.


Diffs
-

  src/tests/master_authorization_tests.cpp 
e43b264b9f67d4cd965aee143cc42a1034ac9952 
  src/tests/master_slave_reconciliation_tests.cpp 
69ec707d4f7c16743a756ad14005cc84a8cdcc54 

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


Testing
---

make check --gtest_repeat=100


Thanks,

Joerg Schad

Re: Review Request 49883: Renamed and lambda-ified a function in allocator tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49610, 49612, 49880, 49883]

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

- Mesos ReviewBot


On July 11, 2016, 9:41 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49883/
> ---
> 
> (Updated July 11, 2016, 9:41 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed and lambda-ified a function in allocator tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49883/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 48593: Refactor Ranges Subtraction.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48593]

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

- Mesos ReviewBot


On July 11, 2016, 7:52 a.m., Yanyan Hu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48593/
> ---
> 
> (Updated July 11, 2016, 7:52 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Guangya Liu, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-5425
> https://issues.apache.org/jira/browse/MESOS-5425
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch reimplement Ranges subtraction using
> IntervalSet data type: Ranges values will be
> converted to IntervalSet values for subtraction
> and the result will be converted back to Ranges
> after subtraction is done. This change is for
> fixing jira MESOS-5425.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/48593/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yanyan Hu
> 
>

Re: Review Request 49612: Removed unnecessary `Clock::settle` calls from test cases.

[Neil Conway]

> On July 5, 2016, 3:08 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3100
> > 
> >
> > I would keep these 3 instances in this test. It's not immediately clear 
> > that `AWAIT` happens in `handleAllocationsAndRecoverResources()`.
> 
> Neil Conway wrote:
> I don't agree with keeping `Clock::settle` in this case. For one thing, 
> it implies the wrong thing: i.e., keeping `Clock::settle` implies that 
> `handleAllocationsAndRecoverResources()` does not do sufficient 
> synchronization on its own, which is incorrect.
> 
> Second, `Clock::settle` is a kludge; it should only be used when there is 
> _no other way_ to achieve the synchronization required by the test. So I'd 
> prefer to discourage its use whenever possible.
> 
> Alexander Rukletsov wrote:
> In general, `Clock::settle` is not a kludge: it allows to make sure a 
> certain event is processed before another event is scheduled.
> 
> It is not the case here, though. We usually write tests in a way that 
> people can easily follow main steps without looking into functions (hence a 
> lot od copy paste code instead of fixture methods). 
> `handleAllocationsAndRecoverResources()` already breaks this convention by 
> modifying the caller's context; removing `Clock::settle` makes things even 
> worse IMO.
> 
> Neil Conway wrote:
> `Clock::settle` is a kludge because it hides the synchronization 
> dependencies between operations. If a test needs to wait for operation X to 
> complete before it does operation X, then X should provide an explicit 
> interface (e.g., return a `Future` or using GMock trickery) to allow the test 
> code to wait for X to complete. `Clock::settle` hides the synchronization 
> dependency between X and Y. Moreover, `Clock::settle` waits for _all_ 
> outstanding messages to be consumed, which is overkill. It is also very easy 
> to write subtly incorrect code using `Clock::settle` -- e.g., look at all the 
> stuff that breaks if you remove the `os::sleep()` call from the 
> `Clock::settle` implementation.
> 
> I agree that `handleAllocationsAndRecoverResources()` is somewhat 
> inconsistent with our usual convention for test cases, but I don't think that 
> the presence/absence of `Clock::settle` makes things any better/worse.
> 
> Alexander Rukletsov wrote:
> Yeah, from this point of view it is a kludge : ). Until we have 
> primitives, that allow us to express operation dependenices in tests, we will 
> have to use `Clock::settle()` and hence educate contributors how to use it 
> properly, especially the difference between performing an action and a 
> corresponding event being put into the actor's mailbox.
> 
> My concern is that with both `Clock::settle()` and `AWAIT_*` absent the 
> dependency between operations is hidden. 
> `handleAllocationsAndRecoverResources()` does not hint that it awaits for 
> allocations to be made. Maybe making it a local lambda and renaming to 
> something like `awaitAllocationsAndRecoverResources()` will help?

Per discussion, renamed the function and made it a lambda: 
https://reviews.apache.org/r/49883/


- Neil


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


On July 5, 2016, 8:51 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49612/
> ---
> 
> (Updated July 5, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case calls `Clock::settle` and then immediately waits for a
> future to be completed, settling the clock is usually unnecessary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49612/diff/
> 
> 
> Testing
> ---
> 
> `mesos-tests --gtest_filter=HierarchicalAllocatorTest.* --gtest_repeat=500 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49883: Renamed and lambda-ified a function in allocator tests.

[Neil Conway]

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

(Updated July 11, 2016, 9:41 a.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Updated to lambda-ify.


Summary (updated)
-

Renamed and lambda-ified a function in allocator tests.


Repository: mesos


Description (updated)
---

Renamed and lambda-ified a function in allocator tests.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 49883: Renamed a function in allocator tests for clarity.

[Neil Conway]

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

Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Renamed a function in allocator tests for clarity.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 49880: Fixed typos in comments in allocator tests.

[Neil Conway]

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

Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Fixed typos in comments in allocator tests.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 
  src/tests/master_allocator_tests.cpp 4beebff9da791c9c80b0bfbe5d0b6f19098f7789 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 49610: Added test case for quota allocation with reserved resources.

[Neil Conway]

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

(Updated July 11, 2016, 8:47 a.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Address review comments.


Repository: mesos


Description
---

This test checks that when setting aside unallocated resources to ensure
that a quota guarantee can be met, we don't use resources that have been
reserved for a different role.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 

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


Testing
---

`./src/mesos-tests 
--gtest_filter="HierarchicalAllocatorTest.QuotaSetAsideReservedResources" 
--gtest_repeat=500 --gtest_break_on_failure` passes. When you apply a patch 
that breaks quota allocation behavior 
(https://gist.github.com/neilconway/97eed34e9b0b312b8511f0f4f57fe061), 
previously `make check` passes, but it fails with this new test added.


Thanks,

Neil Conway

Re: Review Request 48593: Refactor Ranges Subtraction.

[Yanyan Hu]

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

(Updated July 11, 2016, 7:52 a.m.)


Review request for mesos, Alexander Rukletsov, Guangya Liu, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

This patch reimplement Ranges subtraction using
IntervalSet data type: Ranges values will be
converted to IntervalSet values for subtraction
and the result will be converted back to Ranges
after subtraction is done. This change is for
fixing jira MESOS-5425.


Diffs (updated)
-

  src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

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


Testing
---

make check


Thanks,

Yanyan Hu

Re: Review Request 48593: Refactor Ranges Subtraction.

[Yanyan Hu]

> On July 11, 2016, 7:43 a.m., Joris Van Remoortere wrote:
> > Let's use full, and meaningful words for our names.

Yes, will rename res variable to ranges as Joseph suggested. Thanks.


- Yanyan


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


On July 8, 2016, 1:42 a.m., Yanyan Hu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48593/
> ---
> 
> (Updated July 8, 2016, 1:42 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Guangya Liu, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-5425
> https://issues.apache.org/jira/browse/MESOS-5425
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch reimplement Ranges subtraction using
> IntervalSet data type: Ranges values will be
> converted to IntervalSet values for subtraction
> and the result will be converted back to Ranges
> after subtraction is done. This change is for
> fixing jira MESOS-5425.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/48593/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yanyan Hu
> 
>