Re: Review Request 50088: Added mesos-protobuf target to build protobuf libraries.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50088]

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 18, 2016, 10:59 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50088/
> ---
> 
> (Updated July 18, 2016, 10:59 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5852
> https://issues.apache.org/jira/browse/MESOS-5852
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mesos-protobuf target to build protobuf libraries.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
>   src/CMakeLists.txt bde76f7840afe55f20d7551b3f7e5fe522f7f326 
> 
> Diff: https://reviews.apache.org/r/50088/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make mesos-protobufs
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Review Request 50179: Added mesos-logrotate-logger utility executable.

[Srinivas Brahmaroutu]

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

Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added mesos-logrotate-logger utility executable.

This utility executable will allow to test logrotate module through 
modulemanager.


Diffs
-

  src/slave/CMakeLists.txt bb9ad62b2372ca038c43a20d9906aaf43d9ead41 
  src/slave/cmake/SlaveConfigure.cmake ced57496970f1d7edf9e7e443b22d14d2ee948f0 
  src/slave/container_loggers/CMakeLists.txt PRE-CREATION 

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


Testing
---

cmake .. && make mesos-logrotate-logger
GLOG_v=1 ./src/mesos-tests 
--gtest_filter="ContainerLogger*.LOGROTATE_RotateInSandbox" should run 
successfully.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50172: Made `ports` resource configurable in sorter benchmark test.

[Guangya Liu]

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

(Updated 七月 19, 2016, 3:35 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Updated allocated port range to fall into [31000-32000]


Repository: mesos


Description
---

This patch added two new functions to sorter benchmark test to
generate a `ports` resource which falls into a specifed ports
range bounds.

1) fragment: Returns a "ports" resource with the number of ranges.
2) makeRange: Returns the "ports" range bounds.


Diffs (updated)
-

  src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 

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


Testing
---

```
./bin/mesos-tests.sh --benchmark 
--gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1"
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
Using 1000 agents and 50 clients
Added 50 clients in 872us
Added 1000 agents in 26457us
Added allocations for 1000 agents in 79697us
Full sort of 50 clients took 1321us
No-op sort of 50 clients took 28us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (115 ms)
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (115 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (133 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50002, 50003]

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 18, 2016, 10:51 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 18, 2016, 10:51 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50172: Made `ports` resource configurable in sorter benchmark test.

[Benjamin Mahler]

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




src/tests/sorter_tests.cpp (lines 504 - 506)


How about these functions:

```
// Creates a "ports(*)" resource for the given ranges.
Resources makePorts(const Ranges& ranges);

// Fragments the given range into a number of subranges.
// Returns an Error if 'numRanges' is too large.
Try fragment(const Range& range, size_t numRanges);

Range makeRange(uint64_t begin, uint64_t end);
```

The caller will call `makePorts` with the result of `fragment`ing the 
range. This way, the fragment function doesn't have to bundle the logic of



src/tests/sorter_tests.cpp (lines 505 - 506)


The typical format is to wrap the arguments:

```
static Try fragment(
const ::mesos::Value::Range& bounds,
size_t numRanges)
```



src/tests/sorter_tests.cpp (lines 510 - 514)


Based on this comment, the formula is wrong?

10 / 2 + 1 = 6 but the max is 5?



src/tests/sorter_tests.cpp (line 515)


Can you flip this check? It seems to read opposite of how it should: 
numRanges > maxRanges



src/tests/sorter_tests.cpp (lines 516 - 520)


Hm.. maybe we can just keep this simple, in general we don't print the 
caller-available information (bounds and numRanges in this case). Something 
like:

"Requested more distinct ranges than possible"



src/tests/sorter_tests.cpp (lines 534 - 544)


It doesn't seem like you need to special case step == 1, is this case 
possible? I'm pretty confused by this code, I would expect the following 
behavior from fragment:

```
[1-10], 1 -> [1-10]
[1-10], 2 -> [1-4,6-10]
[1-10], 3 -> [1-3,5-6,8-10]
[1-10], 4 -> [1-2,4-5,7-8,10-10] // Multiple choices are possible
[1-10], 5 -> [1-1,3-3,5-5,7-7,9-10] // Multiple choices are possible
```

Or something very simple to implement and still easy to understand, like 
picking off single ranges from the front always, this one has no ambiguity like 
the above:

```
[1-10], 1 -> [1-10]
[1-10], 2 -> [1-1,3-10]
[1-10], 3 -> [1-1,3-3,5-10]
[1-10], 4 -> [1-1,3-3,5-5,7-10]
[1-10], 5 -> [1-1,3-3,5-5,7-7,9-10]
```

But yours does the following from what I can tell:

```
[1-10], 1 -> [1-2]
[1-10], 2 -> [1-2,6-7]
[1-10], 3 -> [1-2,4-5,7-8]
[1-10], 4 -> [1-2,3-4,5-6,7-8] -> [1-8]   // Wrong?
[1-10], 5 -> [1-2,3-4,5-6,7-8,9-10] -> [1-10] // Wrong?
```

This looks wrong..? I'd say let's just implement the second suggestion I 
made here as it's simple, and it is easy to understand.



src/tests/sorter_tests.cpp (line 564)


There is no "ports" associated with this function, since it's just a 
Value::Range, not a Resource. How about just removing this sentence?



src/tests/sorter_tests.cpp (line 632)


Any reason you didn't use s/.get()./->/ here?



src/tests/sorter_tests.cpp (line 636)


Why is it ok to use [1-10] when the agent only has [31000-32000]? Seems 
wrong?



src/tests/sorter_tests.cpp (line 638)


Ditto here, why not use ->?


- Benjamin Mahler


On July 19, 2016, 1:39 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50172/
> ---
> 
> (Updated July 19, 2016, 1:39 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added two new functions to sorter benchmark test to
> generate a `ports` resource which falls into a specifed ports
> range bounds.
> 
> 1) fragment: Returns a "ports" resource with the number of ranges.
> 2) makeRange: Returns the "ports" range bounds.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 
> 
> Diff: https://reviews.apache.org/r/50172/diff/
> 
> 
> Testing
> ---
> 
> ```
> ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test

Re: Review Request 50115: Used `Clock::settle()` to check if the operations are processed.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 18, 2016, 10:16 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50115/
> ---
> 
> (Updated July 18, 2016, 10:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, in HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave,
> we are using `sleep` to check if the operations are processed, this
> is not accurate, we should use `Clock::settle()` instead.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 65c3fe6761ccb1ec7d5fbca9a78b2c62e11885ca 
> 
> Diff: https://reviews.apache.org/r/50115/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 14624us
> Added 1000 agents in 2.253886secs
> Updated 1000 agents in 1.940139secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
>  (4493 ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (4495 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (4520 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 43471: Added the zookeeper patch for the slow add_auth calls.

[haosdent huang]

> On July 18, 2016, 9:05 p.m., Benjamin Mahler wrote:
> > Thanks for tracking down the slowness issue.
> > 
> > I'm a bit hesitant to pull in this patch without it being upstreamed yet. 
> > Have you tried asking on their mailing list if they can pick it back up?

Yes, I sent to 
d...@mesos.apache.org(http://search-hadoop.com/m/JhBoa1alFCg155aSE) before, but 
could not get response.


- haosdent


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


On July 18, 2016, 11:58 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43471/
> ---
> 
> (Updated July 18, 2016, 11:58 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan 
> Xu.
> 
> 
> Bugs: MESOS-4648
> https://issues.apache.org/jira/browse/MESOS-4648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the zookeeper patch for the slow add_auth calls.
> 
> 
> Diffs
> -
> 
>   3rdparty/zookeeper-06d3f3f.patch be2ceaf529895c92dcf53984d6d88c78fb1d74ec 
>   3rdparty/zookeeper-3.4.8.patch 486df1ae96af3426835c9d47ff2e36dd47ccde3f 
> 
> Diff: https://reviews.apache.org/r/43471/diff/
> 
> 
> Testing
> ---
> 
> Before
> 
> ```
> [   OK ] ZooKeeperMasterContenderDetectorTest.NonRetryableFrrors (10053 
> ms)
> [   OK ] MasterZooKeeperTest.MasterInfoAddress (11282 ms)
> [   OK ] ZooKeeperTest.Auth (6688 ms)
> [   OK ] ZooKeeperTest.Create (6690 ms)
> ```
> 
> After
> 
> ```
> [   OK ] ZooKeeperMasterContenderDetectorTest.NonRetryableFrrors (321 ms)
> [   OK ] MasterZooKeeperTest.MasterInfoAddress (447 ms)
> [   OK ] ZooKeeperTest.Auth (233 ms)
> [   OK ] ZooKeeperTest.Create (275 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 50172: Made `ports` resource configurable in sorter benchmark test.

[Guangya Liu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

This patch added two new functions to sorter benchmark test to
generate a `ports` resource which falls into a specifed ports
range bounds.

1) fragment: Returns a "ports" resource with the number of ranges.
2) makeRange: Returns the "ports" range bounds.


Diffs
-

  src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 

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


Testing
---

```
./bin/mesos-tests.sh --benchmark 
--gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1"
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
Using 1000 agents and 50 clients
Added 50 clients in 872us
Added 1000 agents in 26457us
Added allocations for 1000 agents in 79697us
Full sort of 50 clients took 1321us
No-op sort of 50 clients took 28us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (115 ms)
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (115 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (133 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu

Re: Review Request 50163: Fixed a memory lifetime issue in LibeventSSLSocketImp::send.

[Benjamin Mahler]

> On July 18, 2016, 10:01 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, line 715
> > 
> >
> > Let's pull out the side-effect and feed it to the check.

In retrospect it seems ok to in some cases to have side-effects within CHECKs, 
mostly in the case of CHECK_NOTNULL:

```
  evbuffer* buffer = CHECK_NOTNULL(evbuffer_new());
```

Because this reads well and will show the contents of the check 
("evbuffer_new()") in the error message. However, pulling them out for add and 
write_buffer does seem more readable to me:

```
  CHECK_EQ(0, evbuffer_add(buffer, data, size));
  CHECK_EQ(0, bufferevent_write_buffer(self->bev, buffer));

// vs

  int result = evbuffer_add(buffer, data, size);
  CHECK_EQ(0, result);

  int result = bufferevent_write_buffer(self->bev, buffer);
  CHECK_EQ(0, result);
```

So I'll pull them out for these two, but I'll use our CHECK_NOTNULL pattern. 
Sound good?


> On July 18, 2016, 10:01 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 714-715
> > 
> >
> > I think it's ok to do a `CHECK` on the result of `evbuffer_add` if you 
> > test `buffer` for `nullptr` first.
> > We should return a failure if the allocation failed though right?

For CHECK vs Failure, good question! I read their code and saw that the only 
way that evbuffer_add can fail is in the case of us making a programming error, 
or memory allocation failing. Also their documentation only makes a vague 
mention of -1 on error, no error code mechanism is provided, and no error cases 
are documented. :(

So it seems safe to CHECK that this doesn't fail since it's not an expected 
run-time error.


> On July 18, 2016, 10:01 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp, lines 735-736
> > 
> >
> > Why `CHECK` vs `Failure`?

Ditto here w.r.t. not an expected run-time error: from reading their code it 
seems this can only fail via a programmer error or a memory allocation failure. 
If there were any other reasons I would have used a Failure indeed! Wish they 
would document and return error codes.. :(


- Benjamin


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


On July 18, 2016, 9:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50163/
> ---
> 
> (Updated July 18, 2016, 9:40 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function accidentally assumed that 'data' will live beyond the
> scope of the call, by using 'data' within an asynchronous context.
> 
> This copies the data into an 'evbuffer' which will then get moved
> into the output buffer in the event loop. Note that this does not
> introduce an additional copy: we still have a single copy to get
> 'data' into the bufferevent output buffer.
> 
> AFAICT, this bug is currently not triggered from any of the calling
> code because each call-site deletes the 'data' after the Future
> completes (it appears that the 'send_callback' will be executed
> *after* the call to bufferevent_write and after the output buffer
> is flushed).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 2e7f33241b5291593cac4ea4c8f0351c19f7f0c2 
> 
> Diff: https://reviews.apache.org/r/50163/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50163: Fixed a memory lifetime issue in LibeventSSLSocketImp::send.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50163]

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 18, 2016, 9:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50163/
> ---
> 
> (Updated July 18, 2016, 9:40 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function accidentally assumed that 'data' will live beyond the
> scope of the call, by using 'data' within an asynchronous context.
> 
> This copies the data into an 'evbuffer' which will then get moved
> into the output buffer in the event loop. Note that this does not
> introduce an additional copy: we still have a single copy to get
> 'data' into the bufferevent output buffer.
> 
> AFAICT, this bug is currently not triggered from any of the calling
> code because each call-site deletes the 'data' after the Future
> completes (it appears that the 'send_callback' will be executed
> *after* the call to bufferevent_write and after the output buffer
> is flushed).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 2e7f33241b5291593cac4ea4c8f0351c19f7f0c2 
> 
> Diff: https://reviews.apache.org/r/50163/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50177: Add systemd watchdog support.

[Lawrence Wu]

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

(Updated July 19, 2016, 1:14 a.m.)


Review request for mesos, David Robinson and Ian Downes.


Changes
---

add todo


Repository: mesos


Description
---

Adds systemd watchdog support (see 
http://0pointer.de/blog/projects/watchdog.html for context).


Diffs
-

  configure.ac d2136909b7305498ae901a5ea00133142b77f9e6 
  src/linux/systemd.hpp 91134f1d4b100759e45931bd09ca4e1e1aeaaf8a 
  src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 
  src/slave/main.cpp 4624392d30cf391015dcd63f447fe2414a47a16a 

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


Testing (updated)
---

Tested by sending SIGSTOP to running mesos and verifying via journalctl that it 
was killed by the watchdog.

TODO: write actual test by using and forking a mock binary that initializes 
systemd and sending SIGSTOP to that binary. test also needs a unit file for 
that binary and for systemd to run so we can verify that systemd killed it.


File Attachments


mesos gets owned by watchdog
  
https://reviews.apache.org/media/uploaded/files/2016/07/19/3c73b595-d28e-45d4-adfe-697801ba02cc__Screen_Shot_2016-07-18_at_2.09.31_PM.png


Thanks,

Lawrence Wu

Review Request 50177: Add systemd watchdog support.

[Lawrence Wu]

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

Review request for mesos, David Robinson and Ian Downes.


Repository: mesos


Description
---

Adds systemd watchdog support (see 
http://0pointer.de/blog/projects/watchdog.html for context).


Diffs
-

  configure.ac d2136909b7305498ae901a5ea00133142b77f9e6 
  src/linux/systemd.hpp 91134f1d4b100759e45931bd09ca4e1e1aeaaf8a 
  src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 
  src/slave/main.cpp 4624392d30cf391015dcd63f447fe2414a47a16a 

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


Testing
---

Tested by sending SIGSTOP to running mesos and verifying via journalctl that it 
was killed by the watchdog.


File Attachments


mesos gets owned by watchdog
  
https://reviews.apache.org/media/uploaded/files/2016/07/19/3c73b595-d28e-45d4-adfe-697801ba02cc__Screen_Shot_2016-07-18_at_2.09.31_PM.png


Thanks,

Lawrence Wu

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

[Vinod Kone]

> On July 12, 2016, 12:08 a.m., Vinod Kone wrote:
> >

ping?


- Vinod


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


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 50114: Added missing tables attributes in docs.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 17, 2016, 9:26 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50114/
> ---
> 
> (Updated July 17, 2016, 9:26 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Kapil Arya, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Most of the tables presented in docs uses
> `class="table table-striped"`. To make tables
> looks consitent I added this attribute where
> it was missing on modules and authorization.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md fc027ac643906451513e50d166b51c147b4a9d99 
>   docs/modules.md 60191566cf3580792046ba21a6b9a66a21b18e21 
> 
> Diff: https://reviews.apache.org/r/50114/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 49914: Improved the speed of 'MasterAPITest.UnreserveResources'.

[Vinod Kone]

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


Ship it!




This change looks ok to me and indepdendent of the previous 2 reviews in the 
chain. If yes, I'm happy to commit this if you remove the dependency. You might 
be able to discard the first 2 reviews even.

- Vinod Kone


On July 18, 2016, 8:20 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49914/
> ---
> 
> (Updated July 18, 2016, 8:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-5732
> https://issues.apache.org/jira/browse/MESOS-5732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this patch, speed of 'MasterAPITest.UnreserveResources'
> is increased by removing the unnecessary check for unreserved
> resources which is already verified in
> 'MasterAPITest.ReserveResources' and thus by removing the
> delay for waiting for the resource offers to be declined.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 
> 
> Diff: https://reviews.apache.org/r/49914/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50072: Set allocation interval in 'Master Flags' for operator API reservation tests.

[Vinod Kone]

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




src/tests/api_tests.cpp (lines 946 - 1041)


Why this change? The context is not clear from the description or the 
ticket.


- Vinod Kone


On July 18, 2016, 8:16 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50072/
> ---
> 
> (Updated July 18, 2016, 8:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Neil Conway.
> 
> 
> Bugs: MESOS-5725
> https://issues.apache.org/jira/browse/MESOS-5725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set allocation interval in 'Master Flags' for operator
> API reservation tests.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 
> 
> Diff: https://reviews.apache.org/r/50072/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49913: Moved createFrameworkInfo() function definition to tests/mesos.hpp.

[Vinod Kone]

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




src/tests/mesos.hpp (lines 610 - 615)


I think a generic `createFrameworkInfo()` that returns 
DEFAULT_FRAMEWORK_INFO with "role1" is unintuitive for callers. How would they 
know they get "role1" FrameworkInfo?

I wouldn't do this change.


- Vinod Kone


On July 18, 2016, 8:11 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49913/
> ---
> 
> (Updated July 18, 2016, 8:11 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.
> 
> 
> Bugs: MESOS-5725
> https://issues.apache.org/jira/browse/MESOS-5725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There were multiple createFrameworkInfo() definition scattered
> in various testcases. In this patch, only one defintion of
> createFrameworkInfo() is provided in src/tests/mesos.hpp and
> other tests just used this definition.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 2a22f3b0da817e650a25e5e2c951adb7462718b4 
>   src/tests/reservation_endpoints_tests.cpp 
> 48c002d1dc371c285b9421ef5a2c57250d270fa8 
> 
> Diff: https://reviews.apache.org/r/49913/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 18, 2016, 3:26 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50078/
> ---
> 
> (Updated July 18, 2016, 3:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, Till Toenshoff, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5848
> https://issues.apache.org/jira/browse/MESOS-5848
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker executor wraps the health check command into `docker exec`
> and leverages mesos-health-check binary to actually perform the
> check. However prior to this patch there were unnecessary arguments
> passed to the binary, including `path` / `argv[0]` mismatch, log
> message was misleading as well.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 
> 
> Diff: https://reviews.apache.org/r/50078/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 50062: Updated makePortRanges for variable types.

[Benjamin Mahler]

> On July 18, 2016, 7:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 85-114
> > 
> >
> > Looks like we can simplify this like we did in sorter_tests?
> > 
> > ```
> > // Returns a "ports" resource with the number of ranges
> > // specified as: [1-2, 4-5, 7-8, 10-11, ...]
> > static Resource makePortRanges(size_t numRanges)
> > {
> >   ::mesos::Value::Ranges ranges;
> > 
> >   for (size_t i = 0; i < numRanges; ++i) {
> > Value::Range* range = ranges.add_range();
> > range->set_begin((3 * i) + 1);
> > range->set_end(range->begin() + 1);
> >   }
> > 
> >   Value value;
> >   value.set_type(Value::RANGES);
> >   value.mutable_ranges()->CopyFrom(ranges);
> > 
> >   Resource resource;
> >   resource.set_role("*");
> >   resource.set_name("ports");
> >   resource.set_type(Value::RANGES);
> >   resource.mutable_ranges()->CopyFrom(value.ranges());
> > 
> >   return resource;
> > }
> > ```
> > 
> > Then we don't need `makeRange`.
> 
> Guangya Liu wrote:
> Ben, so here I think what your comments for this should be same as the 
> comments you posted on https://reviews.apache.org/r/49843/ now, right?
> 
> ```
> Let's call it something like fragment(...) to make it clearer that it is 
> taking a range and fragmenting it into subranges. Let's also return a Try 
> since there are input cases that cannot be satisfied. E.g. [1-10] can be 
> fragmented into at most 5 ranges [1,3,5,7,9], so fragment([1-10], 6) returns 
> an error.
> ```

Yeah, same comments here


- Benjamin


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


On July 16, 2016, 3:15 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50062/
> ---
> 
> (Updated July 16, 2016, 3:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1) Using `size_t` for counter.
> 2) Using `uint64_t` for range related values.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
> 
> Diff: https://reviews.apache.org/r/50062/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50162: Ignored /etc/hosts if it does not exist in CNI isolator.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On July 19, 2016, 5:58 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50162/
> ---
> 
> (Updated July 19, 2016, 5:58 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-5806
> https://issues.apache.org/jira/browse/MESOS-5806
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored /etc/hosts if it does not exist in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 81121a6a83fa9f62eea06a64ab96b1f564a63da8 
> 
> Diff: https://reviews.apache.org/r/50162/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 50088: Added mesos-protobuf target to build protobuf libraries.

[Srinivas Brahmaroutu]

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

(Updated July 18, 2016, 10:59 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Added mesos-protobuf target to build protobuf libraries.


Diffs (updated)
-

  cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
  src/CMakeLists.txt bde76f7840afe55f20d7551b3f7e5fe522f7f326 

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


Testing
---

cmake .. && make mesos-protobufs


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50062: Updated makePortRanges for variable types.

[Guangya Liu]

> On 七月 18, 2016, 7:07 p.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 85-114
> > 
> >
> > Looks like we can simplify this like we did in sorter_tests?
> > 
> > ```
> > // Returns a "ports" resource with the number of ranges
> > // specified as: [1-2, 4-5, 7-8, 10-11, ...]
> > static Resource makePortRanges(size_t numRanges)
> > {
> >   ::mesos::Value::Ranges ranges;
> > 
> >   for (size_t i = 0; i < numRanges; ++i) {
> > Value::Range* range = ranges.add_range();
> > range->set_begin((3 * i) + 1);
> > range->set_end(range->begin() + 1);
> >   }
> > 
> >   Value value;
> >   value.set_type(Value::RANGES);
> >   value.mutable_ranges()->CopyFrom(ranges);
> > 
> >   Resource resource;
> >   resource.set_role("*");
> >   resource.set_name("ports");
> >   resource.set_type(Value::RANGES);
> >   resource.mutable_ranges()->CopyFrom(value.ranges());
> > 
> >   return resource;
> > }
> > ```
> > 
> > Then we don't need `makeRange`.

Ben, so here I think what your comments for this should be same as the comments 
you posted on https://reviews.apache.org/r/49843/ now, right?

```
Let's call it something like fragment(...) to make it clearer that it is taking 
a range and fragmenting it into subranges. Let's also return a Try since there 
are input cases that cannot be satisfied. E.g. [1-10] can be fragmented into at 
most 5 ranges [1,3,5,7,9], so fragment([1-10], 6) returns an error.
```


- Guangya


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


On 七月 16, 2016, 3:15 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50062/
> ---
> 
> (Updated 七月 16, 2016, 3:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1) Using `size_t` for counter.
> 2) Using `uint64_t` for range related values.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
> 
> Diff: https://reviews.apache.org/r/50062/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Guangya Liu]

> On 七月 14, 2016, 5:56 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp, lines 135-147
> > 
> >
> > This suggests a change of semantics to `Resources`.
> > 
> > Currently: `Resources` always refers to a valid collection resources. 
> > This function would never return Some(Error) unless we change the semantics.
> > 
> > If we want to remove the unnecessary validation, we can do this by 
> > making '`operator += (const Resources& that)`' avoid validating '`that`' 
> > since it's already valid. Currently it just calls '`operator += (const 
> > Resource& r)`' which is unaware of '`r`' already being valid since it comes 
> > from a '`Resources`'.
> > 
> > However, we have to keep the validation in '`operator += (const 
> > Resource& r)`', since '`r`' is just a protobuf coming from an arbitrary 
> > caller, it may be invalid and we need to validate.
> > 
> > Make sense?
> 
> Guangya Liu wrote:
> Hi Ben, does there are any example in mesos code for `Resource` object 
> come from protobuf? I found that we are always using `Resource::parse` to get 
> a `Resource` or `Resources` object and I have added some validation logic in 
> `Resource::parse` in when getting `Resource` or `Resources` object.
> 
> Guangya Liu wrote:
> Ben, got your point. We can set `Resource` object as following:
> 
> ```javascript 
> Resource scalar;
> scalar.set_name("cpus");
> scalar.set_type(Value::SCALAR);
> scalar.mutable_scalar()->set_value(1.234);
> ```
> 
> So if we igore the validation for `'operator += (const Resource& r)'`, 
> there might be some erros if the resource was not constructed correctly. This 
> is not a good way to create `Resource` object but we cannot guard the usage 
> for this now.
> 
> I found that the API `'operator += (const Resource& r)'` was not called 
> directly too frequently, but we are using this API `'operator += (const 
> Resources& that)'` for the most of the time, so seems it is good to keep the 
> validation for `'operator += (const Resource& r)'` as it was not called 
> directly too much. 
> 
> The problem is that `'operator += (const Resources& that)'` is calling 
> `'operator += (const Resource& r)'`, I did not found a good way to keep 
> validation for `'operator += (const Resource& r)'` but ignore the validation 
> for `'operator += (const Resources& that)'`, still checking how we can make 
> this work, any comments/suggestions for this?
> 
> Guangya Liu wrote:
> Did more test by removing validation, the performance of resources 
> operations with `port` resources increased more than 5x.
> 
> With validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 11.212199secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 8.145903secs to perform 1000 'total -= r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 11.154993secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 7.974004secs to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (38490 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (38490 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (38512 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> Without validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 2.90743secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9166us to perform 1000 'total -= r' operations on ports(*):[1-2, 
> 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 3.038083secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9253us to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (5966 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (5966 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5988 ms total)
> [  PASSED  ] 1 test.
> ```

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Guangya Liu]

> On 七月 14, 2016, 5:56 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp, lines 135-147
> > 
> >
> > This suggests a change of semantics to `Resources`.
> > 
> > Currently: `Resources` always refers to a valid collection resources. 
> > This function would never return Some(Error) unless we change the semantics.
> > 
> > If we want to remove the unnecessary validation, we can do this by 
> > making '`operator += (const Resources& that)`' avoid validating '`that`' 
> > since it's already valid. Currently it just calls '`operator += (const 
> > Resource& r)`' which is unaware of '`r`' already being valid since it comes 
> > from a '`Resources`'.
> > 
> > However, we have to keep the validation in '`operator += (const 
> > Resource& r)`', since '`r`' is just a protobuf coming from an arbitrary 
> > caller, it may be invalid and we need to validate.
> > 
> > Make sense?
> 
> Guangya Liu wrote:
> Hi Ben, does there are any example in mesos code for `Resource` object 
> come from protobuf? I found that we are always using `Resource::parse` to get 
> a `Resource` or `Resources` object and I have added some validation logic in 
> `Resource::parse` in when getting `Resource` or `Resources` object.
> 
> Guangya Liu wrote:
> Ben, got your point. We can set `Resource` object as following:
> 
> ```javascript 
> Resource scalar;
> scalar.set_name("cpus");
> scalar.set_type(Value::SCALAR);
> scalar.mutable_scalar()->set_value(1.234);
> ```
> 
> So if we igore the validation for `'operator += (const Resource& r)'`, 
> there might be some erros if the resource was not constructed correctly. This 
> is not a good way to create `Resource` object but we cannot guard the usage 
> for this now.
> 
> I found that the API `'operator += (const Resource& r)'` was not called 
> directly too frequently, but we are using this API `'operator += (const 
> Resources& that)'` for the most of the time, so seems it is good to keep the 
> validation for `'operator += (const Resource& r)'` as it was not called 
> directly too much. 
> 
> The problem is that `'operator += (const Resources& that)'` is calling 
> `'operator += (const Resource& r)'`, I did not found a good way to keep 
> validation for `'operator += (const Resource& r)'` but ignore the validation 
> for `'operator += (const Resources& that)'`, still checking how we can make 
> this work, any comments/suggestions for this?
> 
> Guangya Liu wrote:
> Did more test by removing validation, the performance of resources 
> operations with `port` resources increased more than 5x.
> 
> With validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 11.212199secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 8.145903secs to perform 1000 'total -= r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 11.154993secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 7.974004secs to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (38490 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (38490 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (38512 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> Without validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 2.90743secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9166us to perform 1000 'total -= r' operations on ports(*):[1-2, 
> 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 3.038083secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9253us to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (5966 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (5966 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5988 ms total)
> [  PASSED  ] 1 test.
> ```

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On July 18, 2016, 10:30 p.m., Greg Mann wrote:
> > src/tests/test_framework_test.sh, line 24
> > 
> >
> > I see what you mean about the failure of these `atexit` statements - I 
> > also see lots of litter in my /tmp directory, and in fact, this 
> > TestFramework test was failing for me due to leftover agent metadata in 
> > `/tmp/mesos/local`.
> > 
> > Before modifying this test in this way, I'd like to get to the bottom 
> > of the `atexit` behavior so that this will pass reliably. If you like, I 
> > think you'd be safe getting rid of this part of the diff and leaving the 
> > TestFramework test as it is. I can cover the case where we don't set the 
> > `work_dir` as part of https://issues.apache.org/jira/browse/MESOS-5850

Removed the test change for now.


- Ammar


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


On July 18, 2016, 10:51 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 18, 2016, 10:51 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 49602: Removed a not-useful log message.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On July 18, 2016, 5:53 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49602/
> ---
> 
> (Updated July 18, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a not-useful log message.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/49602/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50115: Used `Clock::settle()` to check if the operations are processed.

[Guangya Liu]

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

(Updated 七月 18, 2016, 10:16 p.m.)


Review request for mesos, Benjamin Mahler, Jie Yu, Klaus Ma, and Jiang Yan Xu.


Repository: mesos


Description
---

Currently, in HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave,
we are using `sleep` to check if the operations are processed, this
is not accurate, we should use `Clock::settle()` instead.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
65c3fe6761ccb1ec7d5fbca9a78b2c62e11885ca 

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


Testing
---

make
make check

```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
Using 1000 agents and 50 frameworks
Added 50 frameworks in 14624us
Added 1000 agents in 2.253886secs
Updated 1000 agents in 1.940139secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1 
(4493 ms)
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (4495 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (4520 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu

Re: Review Request 50163: Fixed a memory lifetime issue in LibeventSSLSocketImp::send.

[Joris Van Remoortere]

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


Fix it, then Ship it!





3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 714 - 715)


I think it's ok to do a `CHECK` on the result of `evbuffer_add` if you test 
`buffer` for `nullptr` first.
We should return a failure if the allocation failed though right?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 715)


Let's pull out the side-effect and feed it to the check.



3rdparty/libprocess/src/libevent_ssl_socket.cpp (lines 735 - 736)


Why `CHECK` vs `Failure`?



3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 736)


Same issue with pulling side-effects out of `CHECK`s.


- Joris Van Remoortere


On July 18, 2016, 9:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50163/
> ---
> 
> (Updated July 18, 2016, 9:40 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This function accidentally assumed that 'data' will live beyond the
> scope of the call, by using 'data' within an asynchronous context.
> 
> This copies the data into an 'evbuffer' which will then get moved
> into the output buffer in the event loop. Note that this does not
> introduce an additional copy: we still have a single copy to get
> 'data' into the bufferevent output buffer.
> 
> AFAICT, this bug is currently not triggered from any of the calling
> code because each call-site deletes the 'data' after the Future
> completes (it appears that the 'send_callback' will be executed
> *after* the call to bufferevent_write and after the output buffer
> is flushed).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 2e7f33241b5291593cac4ea4c8f0351c19f7f0c2 
> 
> Diff: https://reviews.apache.org/r/50163/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50162: Ignored /etc/hosts if it does not exist in CNI isolator.

[Jie Yu]

> On July 18, 2016, 9:47 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1465
> > 
> >
> > shouldn't we just do
> > if (flags.etc_hosts_path.isSome()) {
> > .
> > }
> > 
> > instead of having a separate check fo `flags.etc_hosts_path.isNone()` ?

Just to be consistent with below. This is easier to read IMO :)


- Jie


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


On July 18, 2016, 9:24 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50162/
> ---
> 
> (Updated July 18, 2016, 9:24 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-5806
> https://issues.apache.org/jira/browse/MESOS-5806
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored /etc/hosts if it does not exist in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 81121a6a83fa9f62eea06a64ab96b1f564a63da8 
> 
> Diff: https://reviews.apache.org/r/50162/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 50162: Ignored /etc/hosts if it does not exist in CNI isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1463)


shouldn't we just do
if (flags.etc_hosts_path.isSome()) {
.
}

instead of having a separate check fo `flags.etc_hosts_path.isNone()` ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1533)


Not yours but s/`flags.hostname1/`flags.hostname` ?


- Avinash sridharan


On July 18, 2016, 9:24 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50162/
> ---
> 
> (Updated July 18, 2016, 9:24 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-5806
> https://issues.apache.org/jira/browse/MESOS-5806
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored /etc/hosts if it does not exist in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 81121a6a83fa9f62eea06a64ab96b1f564a63da8 
> 
> Diff: https://reviews.apache.org/r/50162/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 50163: Fixed a memory lifetime issue in LibeventSSLSocketImp::send.

[Benjamin Mahler]

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

Review request for mesos, Joris Van Remoortere and Joseph Wu.


Repository: mesos


Description
---

This function accidentally assumed that 'data' will live beyond the
scope of the call, by using 'data' within an asynchronous context.

This copies the data into an 'evbuffer' which will then get moved
into the output buffer in the event loop. Note that this does not
introduce an additional copy: we still have a single copy to get
'data' into the bufferevent output buffer.

AFAICT, this bug is currently not triggered from any of the calling
code because each call-site deletes the 'data' after the Future
completes (it appears that the 'send_callback' will be executed
*after* the call to bufferevent_write and after the output buffer
is flushed).


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
2e7f33241b5291593cac4ea4c8f0351c19f7f0c2 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 50162: Ignored /etc/hosts if it does not exist in CNI isolator.

[Jie Yu]

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

Review request for mesos, Avinash sridharan and Qian Zhang.


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


Repository: mesos


Description
---

Ignored /etc/hosts if it does not exist in CNI isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
81121a6a83fa9f62eea06a64ab96b1f564a63da8 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 50147: Simplified the checkpointed resources handling logic.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50147]

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 18, 2016, 5:42 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50147/
> ---
> 
> (Updated July 18, 2016, 5:42 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we always checkpoint resources.target and in the agent
> recovery path we have to handle the target being the same as the
> committed resources.info. If we don't create the target when the new
> checkpointed resources are the same, then we don't need to worry about
> that possibility in recovery.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp dc619eb72703d8f168defa120183b2c50ae38d5c 
> 
> Diff: https://reviews.apache.org/r/50147/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 43477: Sped up GroupTest.* test cases by advance clock.

[Benjamin Mahler]

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



Can you reach out to see if Jiang Yan Xu to review this?

- Benjamin Mahler


On July 16, 2016, 2:11 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43477/
> ---
> 
> (Updated July 16, 2016, 2:11 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-4648
> https://issues.apache.org/jira/browse/MESOS-4648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sped up GroupTest.* test cases by advance clock.
> 
> 
> Diffs
> -
> 
>   src/tests/group_tests.cpp 193a158c4d07591584fc64b6baa41f2e90618732 
> 
> Diff: https://reviews.apache.org/r/43477/diff/
> 
> 
> Testing
> ---
> 
> Test repeatly in CentOS 7.1
> 
> ```
>  sudo ./bin/mesos-tests.sh --gtest_filter="GroupTest.*" --verbose 
> --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> Before
> 
> ```
> [   OK ] GroupTest.GroupJoinWithDisconnect (3352 ms)
> [   OK ] GroupTest.GroupDataWithDisconnect (3350 ms)
> [   OK ] GroupTest.GroupCancelWithDisconnect (2013 ms)
> [   OK ] GroupTest.GroupPathWithRestrictivePerms (13368 ms)
> [   OK ] GroupTest.RetryableErrors (26720 ms)
> ```
> 
> After 
> 
> ```
> [   OK ] GroupTest.GroupJoinWithDisconnect (405 ms)
> [   OK ] GroupTest.GroupDataWithDisconnect (192 ms)
> [   OK ] GroupTest.GroupCancelWithDisconnect (250 ms)
> [   OK ] GroupTest.GroupPathWithRestrictivePerms (334 ms)
> [   OK ] GroupTest.RetryableErrors (341 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43474: Sped up MasterDetectorExpireSlaveZKSession by advance Clock.

[Benjamin Mahler]

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



Can you reach out to see if Jiang Yan Xu to review this?

- Benjamin Mahler


On July 16, 2016, 2:10 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43474/
> ---
> 
> (Updated July 16, 2016, 2:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-4651
> https://issues.apache.org/jira/browse/MESOS-4651
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sped up MasterDetectorExpireSlaveZKSession by advance Clock.
> 
> 
> Diffs
> -
> 
>   src/tests/master_contender_detector_tests.cpp 
> 2970986830d0cfaedbf17f3b002c40ac5c28bd2a 
> 
> Diff: https://reviews.apache.org/r/43474/diff/
> 
> 
> Testing
> ---
> 
> Test repeatly in CentOS 7.1
> 
> ```
>  sudo ./bin/mesos-tests.sh 
> --gtest_filter="ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSession"
>  --verbose --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> Before
> 
> ```
> [   OK ] 
> ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSession (3358 
> ms)
> ```
> 
> After 
> 
> ```
> [   OK ] 
> ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSession (448 
> ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43475: Sped up MasterDetectorExpireSlaveZKSessionNewMaster by advance Clock.

[Benjamin Mahler]

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



Can you reach out to see if Jiang Yan Xu to review this?

- Benjamin Mahler


On July 16, 2016, 2:11 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43475/
> ---
> 
> (Updated July 16, 2016, 2:11 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-4652
> https://issues.apache.org/jira/browse/MESOS-4652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sped up MasterDetectorExpireSlaveZKSessionNewMaster by advance Clock.
> 
> 
> Diffs
> -
> 
>   src/tests/master_contender_detector_tests.cpp 
> 2970986830d0cfaedbf17f3b002c40ac5c28bd2a 
> 
> Diff: https://reviews.apache.org/r/43475/diff/
> 
> 
> Testing
> ---
> 
> Test repeatly in CentOS 7.1
> 
> ```
>  sudo ./bin/mesos-tests.sh 
> --gtest_filter="ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSessionNewMaster"
>  --verbose --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> Before
> ```
> [   OK ] 
> ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSessionNewMaster
>  (3359 ms)
> ```
> 
> After
> 
> ```
> [   OK ] 
> ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSessionNewMaster
>  (613 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43473: Sped up ContenderDetectorShutdownNetwork by advance Clock.

[Benjamin Mahler]

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



Can you reach out to see if Jiang Yan Xu to review this? He wrote many of these 
tests.

- Benjamin Mahler


On July 16, 2016, 2:09 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43473/
> ---
> 
> (Updated July 16, 2016, 2:09 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-4650
> https://issues.apache.org/jira/browse/MESOS-4650
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sped up ContenderDetectorShutdownNetwork by advance Clock.
> 
> 
> Diffs
> -
> 
>   src/tests/master_contender_detector_tests.cpp 
> 2970986830d0cfaedbf17f3b002c40ac5c28bd2a 
> 
> Diff: https://reviews.apache.org/r/43473/diff/
> 
> 
> Testing
> ---
> 
> Test repeatly in CentOS 7.1
> 
> ```
>  sudo ./bin/mesos-tests.sh 
> --gtest_filter="ZooKeeperMasterContenderDetectorTest.ContenderDetectorShutdownNetwork
>  " --verbose --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> Before
> 
> ```
> [   OK ] 
> ZooKeeperMasterContenderDetectorTest.ContenderDetectorShutdownNetwork (3390 
> ms)
> ```
> 
> After
> 
> ```
> [   OK ] 
> ZooKeeperMasterContenderDetectorTest.ContenderDetectorShutdownNetwork (546 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43472: Sped up ZooKeeperTest.LeaderContender by advance Clock.

[Benjamin Mahler]

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



Can you reach out to see if Jiang Yan Xu to review this? He wrote many of these 
tests.

- Benjamin Mahler


On July 16, 2016, 2:09 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43472/
> ---
> 
> (Updated July 16, 2016, 2:09 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-4649
> https://issues.apache.org/jira/browse/MESOS-4649
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sped up ZooKeeperTest.LeaderContender by advance Clock.
> 
> 
> Diffs
> -
> 
>   src/tests/zookeeper_tests.cpp 72279a3397af7fa15f6ecb3c7adb9d149439e8c8 
> 
> Diff: https://reviews.apache.org/r/43472/diff/
> 
> 
> Testing
> ---
> 
> Test repeatly in CentOS 7.1
> 
> ```
>  sudo ./bin/mesos-tests.sh --gtest_filter="ZooKeeperTest.LeaderContender" 
> --verbose --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> Before
> 
> ```
> [   OK ] ZooKeeperTest.LeaderContender (3385 ms)
> ```
> 
> After
> 
> ```
> [   OK ] ZooKeeperTest.LeaderContender (694 ms)
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49843: Added benchmark test for sorter.

[Benjamin Mahler]

> On July 14, 2016, 7:14 p.m., Benjamin Mahler wrote:
> > src/tests/sorter_tests.cpp, lines 504-542
> > 
> >
> > It looks like we can simplify these two functions down to the following 
> > single function?
> > 
> > ```
> > // Returns a "ports" resource with the number of ranges
> > // specified as: [1-2, 4-5, 7-8, 10-11, ...]
> > static Resource makePortRanges(size_t numRanges)
> > {
> >   ::mesos::Value::Ranges ranges;
> > 
> >   for (size_t i = 0; i < numRanges; ++i) {
> > Value::Range* range = ranges.add_range();
> > range->set_begin((3 * i) + 1);
> > range->set_end(range->begin() + 1);
> >   }
> > 
> >   Value value;
> >   value.set_type(Value::RANGES);
> >   value.mutable_ranges()->CopyFrom(ranges);
> > 
> >   Resource resource;
> >   resource.set_role("*");
> >   resource.set_name("ports");
> >   resource.set_type(Value::RANGES);
> >   resource.mutable_ranges()->CopyFrom(value.ranges());
> > 
> >   return resource;
> > }
> > ```
> > 
> > Since we only care about the number of ranges, we don't need to specify 
> > the bounds, right?
> 
> Guangya Liu wrote:
> Yes, my orignial thinking was keep this same as 
> https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L85-L122
>  in case we may need specify bounds in future, but since this is only for 
> test and we do not need to care for bounds, so seems no need to have 
> `makeRange` here. I was thinking we may also need to update 
> https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L85-L122
>  as well to use a single function to create port ranges.
> 
> Guangya Liu wrote:
> Hi Ben, there is one issue for current function: We are now using port 
> range as `[1-2, 4-5, 7-8, 10-11, ...]`, but the slave created using port 
> `[31000-32000]` in here 
> https://github.com/apache/mesos/blob/master/src/tests/sorter_tests.cpp#L565-L566
>  . 
> 
> Basically, clients cannot get port resources which are not in 
> `[31000-32000]` but here we are using port range `[1-2, 4-5, 7-8, 10-11, 
> ...]`, this will not impact the `sorter` result as the port resources are 
> excluded from sort 
> https://github.com/apache/mesos/blob/master/src/master/allocator/sorter/drf/sorter.cpp#L288
>  .
> 
> But I think we still need to fix this to make sure the logic tecnically 
> right here, there are two options: 1) Keep the orignial code and have two 
> functions by using `makeRange` to define the bounds. 2) Hard code the 
> resources in slave and make the port range starting from 1. I think 1) is 
> better as this can make the port range controllable via some parameters, and 
> for 2), we need to make sure the allocated port range always fall into the 
> agent resources port range when setting the agent resources.
> 
> What do you think?

Now I see why there were two functions! Sorry about that.

Let's call it something like `fragment(...)` to make it clearer that it is 
taking a range and fragmenting it into subranges. Let's also return a Try since 
there are input cases that cannot be satisfied. E.g. [1-10] can be fragmented 
into at most 5 ranges [1,3,5,7,9], so `fragment([1-10], 6)` returns an error.


- Benjamin


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


On July 12, 2016, 9:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49843/
> ---
> 
> (Updated July 12, 2016, 9:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-5701
> https://issues.apache.org/jira/browse/MESOS-5701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added benchmark test for sorter.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp 20e42419934e81b97676569876cd63ee0a550da3 
> 
> Diff: https://reviews.apache.org/r/49843/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
>  
>  ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/*"
> [==] Running 36 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 36 tests from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0
> Using 1000 agents and 1 clients
> Added 1 clients in 1047us
> Added 1000 agents in 30147us
> Sorted 1 clients in 87us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0 (33 ms)
> [ RUN  ] 

Re: Review Request 50138: Synced up common/values.cpp and v1/values.cpp.

[Joseph Wu]

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


Ship it!




Thanks for doing this :)

- Joseph Wu


On July 18, 2016, 7:57 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50138/
> ---
> 
> (Updated July 18, 2016, 7:57 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix synced up two patches from common/values.cpp to
> v1/values.cpp.
> 
> 1) https://reviews.apache.org/r/45813/ Removed capture by
>reference.
> 2) https://reviews.apache.org/r/48593/ Refactor Value::Ranges
>subtraction.
> 
> 
> Diffs
> -
> 
>   src/v1/values.cpp 3e0f739b69e680f9eb29ed36d4501c393758c861 
> 
> Diff: https://reviews.apache.org/r/50138/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50138: Synced up common/values.cpp and v1/values.cpp.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50138]

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 18, 2016, 2:57 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50138/
> ---
> 
> (Updated July 18, 2016, 2:57 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix synced up two patches from common/values.cpp to
> v1/values.cpp.
> 
> 1) https://reviews.apache.org/r/45813/ Removed capture by
>reference.
> 2) https://reviews.apache.org/r/48593/ Refactor Value::Ranges
>subtraction.
> 
> 
> Diffs
> -
> 
>   src/v1/values.cpp 3e0f739b69e680f9eb29ed36d4501c393758c861 
> 
> Diff: https://reviews.apache.org/r/50138/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Benjamin Mahler]

> On July 14, 2016, 5:56 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp, lines 135-147
> > 
> >
> > This suggests a change of semantics to `Resources`.
> > 
> > Currently: `Resources` always refers to a valid collection resources. 
> > This function would never return Some(Error) unless we change the semantics.
> > 
> > If we want to remove the unnecessary validation, we can do this by 
> > making '`operator += (const Resources& that)`' avoid validating '`that`' 
> > since it's already valid. Currently it just calls '`operator += (const 
> > Resource& r)`' which is unaware of '`r`' already being valid since it comes 
> > from a '`Resources`'.
> > 
> > However, we have to keep the validation in '`operator += (const 
> > Resource& r)`', since '`r`' is just a protobuf coming from an arbitrary 
> > caller, it may be invalid and we need to validate.
> > 
> > Make sense?
> 
> Guangya Liu wrote:
> Hi Ben, does there are any example in mesos code for `Resource` object 
> come from protobuf? I found that we are always using `Resource::parse` to get 
> a `Resource` or `Resources` object and I have added some validation logic in 
> `Resource::parse` in when getting `Resource` or `Resources` object.
> 
> Guangya Liu wrote:
> Ben, got your point. We can set `Resource` object as following:
> 
> ```javascript 
> Resource scalar;
> scalar.set_name("cpus");
> scalar.set_type(Value::SCALAR);
> scalar.mutable_scalar()->set_value(1.234);
> ```
> 
> So if we igore the validation for `'operator += (const Resource& r)'`, 
> there might be some erros if the resource was not constructed correctly. This 
> is not a good way to create `Resource` object but we cannot guard the usage 
> for this now.
> 
> I found that the API `'operator += (const Resource& r)'` was not called 
> directly too frequently, but we are using this API `'operator += (const 
> Resources& that)'` for the most of the time, so seems it is good to keep the 
> validation for `'operator += (const Resource& r)'` as it was not called 
> directly too much. 
> 
> The problem is that `'operator += (const Resources& that)'` is calling 
> `'operator += (const Resource& r)'`, I did not found a good way to keep 
> validation for `'operator += (const Resource& r)'` but ignore the validation 
> for `'operator += (const Resources& that)'`, still checking how we can make 
> this work, any comments/suggestions for this?
> 
> Guangya Liu wrote:
> Did more test by removing validation, the performance of resources 
> operations with `port` resources increased more than 5x.
> 
> With validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 11.212199secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 8.145903secs to perform 1000 'total -= r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 11.154993secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 7.974004secs to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (38490 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (38490 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (38512 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> Without validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 2.90743secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9166us to perform 1000 'total -= r' operations on ports(*):[1-2, 
> 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 3.038083secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9253us to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (5966 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (5966 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5988 ms total)
> [  PASSED  ] 1 test.
> 

Re: Review Request 50117: Added more elapse time for allocator benchmark test.

[Benjamin Mahler]

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


Ship it!




Nice patch, thanks!

- Benjamin Mahler


On July 18, 2016, 5:29 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50117/
> ---
> 
> (Updated July 18, 2016, 5:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is adding elapse time for addFramework, addSlave
> and setQuota in allocator benchmark test, adding those elapse
> time can help evaluate the performnace of those APIs.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
> 
> Diff: https://reviews.apache.org/r/50117/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> DeclineOffers benchmark
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 14779us
> Added 1000 agents in 2.7833secs
> round 0 allocate() took 2.325974secs to make 1000 offers
> round 1 allocate() took 2.349536secs to make 1000 offers
> round 2 allocate() took 2.368871secs to make 1000 offers
> round 3 allocate() took 2.584638secs to make 1000 offers
> ```
> 
> ResourceLabels benchmark
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 12574us
> Added 1000 agents in 3.37232secs
> round 0 allocate() took 2.459538secs to make 1000 offers
> round 1 allocate() took 2.474587secs to make 1000 offers
> ```
> 
> Mtrics benchmark
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/1
> Set quota for 50 roles in 64060us
> Added 50 frameworks in 97191us
> Added 1000 agents in 9.80secs
> /metrics/snapshot took 78252us for 1000 agents and 50 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/1 (10055 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (10055 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (10079 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50080: Updated Windows build instructions.

[Joseph Wu]

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

(Updated July 18, 2016, 12:14 p.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
Vinod Kone.


Changes
---

Updated binary paths due to the change here: https://reviews.apache.org/r/49178/


Repository: mesos


Description
---

Adds notes about the type of Visual Studio installation expected (C++),
the git checkout configuration, and the build path on Windows.


Diffs (updated)
-

  docs/getting-started.md 104816ed4be06ab6be96de9d69cf2b01d60199d0 

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


Testing
---

Visually confirmed (via website preview) that the markdown is sane.


Thanks,

Joseph Wu

Re: Review Request 50115: Used `Clock::settle()` to check if the operations are processed.

[Benjamin Mahler]

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




src/tests/hierarchical_allocator_tests.cpp (lines 3325 - 3326)


Good fix, thanks!



src/tests/hierarchical_allocator_tests.cpp (lines 3354 - 3355)


Replacing sleep with settle sounds good! Why not keep the 'finished' 
variable so that we can ensure the settle is doing what we expect?

```
  // Wait for all the `addSlave` operations to be processed.
  Clock::settle();

  ASSERT_EQ(slaveCount, offerCallbacks.load());
```

Note that 'finished' is a bit vague, perhaps offerCallbacks is clearer?



src/tests/hierarchical_allocator_tests.cpp (lines 3372 - 3373)


Ditto here:

```
```
  // Wait for all the `updateSlave` operations to be processed.
  Clock::settle();

  ASSERT_EQ(slaveCount * 2, offerCallbacks.load());
```
```


- Benjamin Mahler


On July 18, 2016, 3:32 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50115/
> ---
> 
> (Updated July 18, 2016, 3:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, in HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave,
> we are using `sleep` to check if the operations are processed, this
> is not accurate, we should use `Clock::settle()` instead.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
> 
> Diff: https://reviews.apache.org/r/50115/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 14624us
> Added 1000 agents in 2.253886secs
> Updated 1000 agents in 1.940139secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
>  (4493 ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (4495 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (4520 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50062: Updated makePortRanges for variable types.

[Benjamin Mahler]

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




src/tests/hierarchical_allocator_tests.cpp (lines 85 - 114)


Looks like we can simplify this like we did in sorter_tests?

```
// Returns a "ports" resource with the number of ranges
// specified as: [1-2, 4-5, 7-8, 10-11, ...]
static Resource makePortRanges(size_t numRanges)
{
  ::mesos::Value::Ranges ranges;

  for (size_t i = 0; i < numRanges; ++i) {
Value::Range* range = ranges.add_range();
range->set_begin((3 * i) + 1);
range->set_end(range->begin() + 1);
  }

  Value value;
  value.set_type(Value::RANGES);
  value.mutable_ranges()->CopyFrom(ranges);

  Resource resource;
  resource.set_role("*");
  resource.set_name("ports");
  resource.set_type(Value::RANGES);
  resource.mutable_ranges()->CopyFrom(value.ranges());

  return resource;
}
```

Then we don't need `makeRange`.


- Benjamin Mahler


On July 16, 2016, 3:15 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50062/
> ---
> 
> (Updated July 16, 2016, 3:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1) Using `size_t` for counter.
> 2) Using `uint64_t` for range related values.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
> 
> Diff: https://reviews.apache.org/r/50062/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50060: Always call watch.stop() to get an accurate time for benchmark test.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 15, 2016, 2:27 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50060/
> ---
> 
> (Updated July 15, 2016, 2:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This path include two fixes:
> 1) Always call watch.stop() explicitly to get an accurate time
> for benchmark test.
> 2) Move `watch.start()` right before `allocator->addSlave` to
> make the time more accurate.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 153c9b4cf4819e976910c5a7ad9602028e2d22eb 
> 
> Diff: https://reviews.apache.org/r/50060/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="*HierarchicalAllocator_BENCHMARK_Test.*/1"
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50147: Simplified the checkpointed resources handling logic.

[Anindya Sinha]

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


Fix it, then Ship it!





src/slave/slave.cpp (line 2599)


s/the the/the/g. There are at least 2 such instances.



src/slave/slave.cpp (line 2600)


Nitpik: s/it's/it is


- Anindya Sinha


On July 18, 2016, 5:42 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50147/
> ---
> 
> (Updated July 18, 2016, 5:42 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we always checkpoint resources.target and in the agent
> recovery path we have to handle the target being the same as the
> committed resources.info. If we don't create the target when the new
> checkpointed resources are the same, then we don't need to worry about
> that possibility in recovery.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp dc619eb72703d8f168defa120183b2c50ae38d5c 
> 
> Diff: https://reviews.apache.org/r/50147/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 49602: Removed a not-useful log message.

[Neil Conway]

> On July 15, 2016, 4:26 p.m., Greg Mann wrote:
> > Neil I agree - not useful logging information :-) Shall we just remove it 
> > instead?
> 
> Anand Mazumdar wrote:
> +1 to killing the log message.

Sounds good, updated the review.


- Neil


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


On July 18, 2016, 5:53 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49602/
> ---
> 
> (Updated July 18, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a not-useful log message.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/49602/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49602: Removed a not-useful log message.

[Neil Conway]

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

(Updated July 18, 2016, 5:53 p.m.)


Review request for mesos, Anand Mazumdar and Greg Mann.


Changes
---

Remove log message instead.


Summary (updated)
-

Removed a not-useful log message.


Repository: mesos


Description (updated)
---

Removed a not-useful log message.


Diffs (updated)
-

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

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


Testing (updated)
---

make check


Thanks,

Neil Conway

Re: Review Request 50147: Simplified the checkpointed resources handling logic.

[Jiang Yan Xu]

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

(Updated July 18, 2016, 10:42 a.m.)


Review request for mesos, Anindya Sinha and Neil Conway.


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


Repository: mesos


Description (updated)
---

Currently we always checkpoint resources.target and in the agent
recovery path we have to handle the target being the same as the
committed resources.info. If we don't create the target when the new
checkpointed resources are the same, then we don't need to worry about
that possibility in recovery.


Diffs
-

  src/slave/slave.cpp dc619eb72703d8f168defa120183b2c50ae38d5c 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 50147: Simplified the checkpoint resources handling logic.

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha and Neil Conway.


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


Repository: mesos


Description
---

Currently we always checkpoint resources.target and in the agent
recovery path we have to handle the target being the same as the
committed  resources.info. If we don't create a when the new
checkpointed resources are the same, then we don't need to go through
that additional handling.


Diffs
-

  src/slave/slave.cpp dc619eb72703d8f168defa120183b2c50ae38d5c 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 50147: Simplified the checkpointed resources handling logic.

[Jiang Yan Xu]

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

(Updated July 18, 2016, 10:40 a.m.)


Review request for mesos, Anindya Sinha and Neil Conway.


Summary (updated)
-

Simplified the checkpointed resources handling logic.


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


Repository: mesos


Description
---

Currently we always checkpoint resources.target and in the agent
recovery path we have to handle the target being the same as the
committed  resources.info. If we don't create a when the new
checkpointed resources are the same, then we don't need to go through
that additional handling.


Diffs
-

  src/slave/slave.cpp dc619eb72703d8f168defa120183b2c50ae38d5c 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 49571: Added a benchmark test for allocations and shared resources.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45959, 48616, 45960, 45961, 45962, 45963, 45964, 45966, 
45967, 49571]

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 18, 2016, 2:31 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated July 18, 2016, 2:31 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocations test has the following configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> Added a case for testing arithmetic operations on shared resources.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> Resource arithmetic benchmark test results
> ==
> Minimal impact seen in Resources arithmetic with the Resources refactor 
> changes to incorporate shared resources.
> 
> With shared resources patch (note that 4th test below is for shared resources 
> for scalars)
> --
> 
> [--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0 (980 ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1 (17128 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (9217 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3 (1100 
> ms)
> [--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test (28425 
> ms total)
> 
> HEAD
> 
> 
> [--] 3 tests from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0 (866 ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1 (17563 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (9218 
> ms)
> [--] 3 tests from ResourcesOperators/Resources_BENCHMARK_Test (27647 
> ms total)
> 
> Allocations benchmark test results
> ==
> Support of shared resources has a small impact on runtime performance in 
> allocations. Also, there is no visible impact in performance when shared 
> resources are added in the tests.
> 
> With the patch (and no shared resources)
> 
> round 0 allocate took 3.19704secs to make 200 offers
> round 50 allocate took 3.240605secs to make 200 offers
> round 100 allocate took 3.227024secs to make 200 offers
> round 150 allocate took 3.225281secs to make 200 offers
> round 199 allocate took 3.26036secs to make 200 offers
> 
> With the patch (and shared resources on all agents)
> ---
> round 0 allocate took 3.279115secs to make 200 offers
> round 50 allocate took 3.273396secs to make 200 offers
> round 100 allocate took 3.278509secs to make 200 offers
> round 150 allocate took 3.275959secs to make 200 offers
> round 199 allocate took 3.278151secs to make 200 offers
> 
> With the patch (and shared resources on alternate agents)
> -
> round 0 allocate took 3.251739secs to make 200 offers
> round 50 allocate took 3.263777secs to make 200 offers
> round 100 allocate took 3.263079secs to make 200 offers
> round 150 allocate took 3.263114secs to make 200 offers
> 

Re: Review Request 50145: Updated the 1.0.0 CHANGELOG for a bug fix.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 18, 2016, 4:50 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50145/
> ---
> 
> (Updated July 18, 2016, 4:50 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the 1.0.0 CHANGELOG for a bug fix.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fee129c6bdfc16d1ac038a23b4b1b097203a1502 
> 
> Diff: https://reviews.apache.org/r/50145/diff/
> 
> 
> Testing
> ---
> 
> trivial
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 50145: Updated the 1.0.0 CHANGELOG for a bug fix.

[Jie Yu]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Updated the 1.0.0 CHANGELOG for a bug fix.


Diffs
-

  CHANGELOG fee129c6bdfc16d1ac038a23b4b1b097203a1502 

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


Testing
---

trivial


Thanks,

Jie Yu

Review Request 50138: Synced up common/values.cpp and v1/values.cpp.

[Guangya Liu]

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

Review request for mesos, Joris Van Remoortere and Joseph Wu.


Repository: mesos


Description
---

This fix synced up two patches from common/values.cpp to
v1/values.cpp.

1) https://reviews.apache.org/r/45813/ Removed capture by
   reference.
2) https://reviews.apache.org/r/48593/ Refactor Value::Ranges
   subtraction.


Diffs
-

  src/v1/values.cpp 3e0f739b69e680f9eb29ed36d4501c393758c861 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:31 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Added a new capability SHARED_RESOURCES that frameworks need to opt
in if they are interested in receiving shared resources in their
offers.


Diffs (updated)
-

  include/mesos/mesos.proto 7796c72694808618a1b0af2e7d00111f9674785c 
  include/mesos/v1/mesos.proto fd03c540a819ea43ccb2965600d063f9b736a8ba 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/allocator/mesos/hierarchical.hpp 
b72ba16277a3210e4d309b616d185a10e2029a66 
  src/master/allocator/mesos/hierarchical.cpp 
7d4064535a20b93950f5a95eef1ad3f0d37d305b 
  src/tests/hierarchical_allocator_tests.cpp 
ce5da6be490b6fce311286eb4018c91eef55067e 

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


Testing
---

Tests updated with new capability.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45967: Added documentation for shareable resources.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:31 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md 5e6225ce9fd5a3fab817ec333fc5d45fb4488956 
  docs/shared-resources.md PRE-CREATION 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45964: Add unit tests for sharing of resources.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:31 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Add unit tests for sharing of resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
ce5da6be490b6fce311286eb4018c91eef55067e 
  src/tests/master_validation_tests.cpp 
9eb82a569f7c48caa96d4d54a93b199ccac74385 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/persistent_volume_tests.cpp 
a6f97c4bb5fb29d610c01255036095e2b30c44c5 
  src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 

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


Testing
---

Tests for shared resources added for allocator, resources and sorter.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:31 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, allow the task to be executed only if the ownership of the
persistent volume matches that of the task.
Added an option to run the test in mixed (default) mode or shared-only
mode. In mixed mode, multiple shards alternate between shared and
unshared persistent volumes for the tasks. In shared-only mode, all
shards use shared persistent volumes for the tasks.


Diffs (updated)
-

  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
0afe9272900cfa4b39887eb259070a9a2df2ab93 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
db3ed8f69de8b52633194b252b0e5aba38ec69c0 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
794b6e5990db5f8eb21a6535872f284ca02e0553 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
0809e8ec35232fbdafee171a7a960cbdec272134 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45961: Support sharing of resources through reference counting of resources.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:31 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael Park, 
and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

o Each shared resource is accouted via its share count. This count is
  updated based on the resource operations (such as add and subtract)
  in various scenarios such as task launch and terminate at multiple
  modules such as master, allocator, sorter, etc.
o Only allow DESTROY if there are no running or pending tasks using
  the volume. However, if the volume is in a pending offer to one or
  more frameworks, resind that offer before processing the DESTROY.


Diffs (updated)
-

  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d 
  src/master/allocator/mesos/hierarchical.hpp 
b72ba16277a3210e4d309b616d185a10e2029a66 
  src/master/allocator/mesos/hierarchical.cpp 
7d4064535a20b93950f5a95eef1ad3f0d37d305b 
  src/master/allocator/sorter/drf/sorter.hpp 
bc6bfb2d5d3b32d55be055a0514861b4e7d889bb 
  src/master/allocator/sorter/drf/sorter.cpp 
ac85b327fc33d34246788e6a8c8bf5a486c61434 
  src/master/http.cpp 0293e4956afb5968f84b516e3d4b5f6f858ef7a3 
  src/master/master.hpp bade8af69f567341667b9907368207189d0dab0e 
  src/master/master.cpp 370fd8712062dc75bb81824cb99ccc7920acbf78 
  src/master/quota_handler.cpp bf6a613a7bb3c62fd77d1ffde3170749d6c21fa2 
  src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/master/weights_handler.cpp 9a901e71ba2fbf2ca1c02f658a72d44cfaa5ec62 
  src/tests/master_validation_tests.cpp 
9eb82a569f7c48caa96d4d54a93b199ccac74385 
  src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45960: Added interfaces to handle and track shareable resources.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:29 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Added interfaces to handle and track shareable resources.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 48616: Add v1 changes for shared resources.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:29 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Add v1 changes for shared resources.


Diffs (updated)
-

  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

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

(Updated July 18, 2016, 2:29 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

A new class Resource_ is added that allows 'Resources' to group
identical shared resource objects together into a single 'Resource_'
object and tracked by its shared count. Non-shared resource objects
are not grouped.

For resource addition and subtraction, the shared count is adjusted for
shared resources as follows:
a) Addition: If shared resource is absent from original, then the
   resource is added initialized with a consumer count of 1. Otherwise,
   the share count for the shared resource is incremented.
b) Subtraction: If shared resource's share count is already 1, then
   the shared resource is removed from the original. Otherwise, its
   consumer count is decremented.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 

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


Testing
---

New tests added to demonstrate arithmetic operations for shared resources with 
consumer counts.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Guangya Liu]

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

(Updated 七月 18, 2016, 2:24 p.m.)


Review request for mesos, Benjamin Mahler and Klaus Ma.


Repository: mesos


Description (updated)
---

The "validation" API was called in a huge number of times, but this
was only needed when parsing resources and we do not need to do
other validation when doing resources operations, such as add etc.

This patch is a WIP patch and was now only removing the `validation`
from `operator+=` and `operator-=` for `Resource`.

Now there are 4 places where we can constrcut resoures.

1) From framework, the resources here will be validated when launching
task, so it is ok even if the framework developer made some mistake to
construct some invalid resources.
2) From agent flags, we can validate this resource when parsing it.
3) From agent resources estimator, we can validate those resources when
got those resources from estimator in agent. We need to update agent code 
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L5035
to validate the resources.
4) From agent resources hook, we can validate this in hook manager when
got those resoures from resources hooks. We need to update agent code here 
https://github.com/apache/mesos/blob/master/src/hook/manager.cpp#L366
to validate the resources.

After the above finished, we can simply remove the `validate()` from
`operator+=` and `operator-=` for `Resource`


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing (updated)
---

The `qcachegrind` result here: 
https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing

Sorter test, the sorter add allocation increased 10x.

With validation:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
Using 5 agents and 1000 clients
Added 1000 clients in 19572us
Added 5 agents in 915932us
Added allocations for 5 agents in 40.303108secs
Full sort of 1000 clients took 32702us
No-op sort of 1000 clients took 314us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (42449 ms)
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (42449 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (42466 ms total)
[  PASSED  ] 1 test.
```

Without validation:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
Using 5 agents and 1000 clients
Added 1000 clients in 22764us
Added 5 agents in 862959us
Added allocations for 5 agents in 3.934831secs
Full sort of 1000 clients took 30187us
No-op sort of 1000 clients took 322us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (6031 ms)
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (6031 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (6049 ms total)
[  PASSED  ] 1 test.
```

The performance of resources operations with port resources operations.

With validation
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
Took 11.212199secs to perform 1000 'total += r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 8.145903secs to perform 1000 'total -= r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 11.154993secs to perform 1000 'total = total + r' operations on 
ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 7.974004secs to perform 1000 'total = total - r' operations on 
ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (38490 ms)
[--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test (38490 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (38512 ms total)
[  PASSED  ] 1 test.
```

Without validation
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 

Re: Review Request 50128: Added Parse() and Serialize() to Docker::Device.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50123, 50124, 50125, 50126, 50127, 50128]

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 18, 2016, 9:35 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50128/
> ---
> 
> (Updated July 18, 2016, 9:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wrapped Parse() and Serialize() functions to Docker::Device structure.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 515842d381ca8a91ad481f66c7be057dff2f3f28 
>   src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 
>   src/slave/containerizer/docker.cpp f1ecf3b25d85597f6c3dcaa47968860ed119dbd5 
> 
> Diff: https://reviews.apache.org/r/50128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Guangya Liu]

> On 七月 14, 2016, 5:56 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp, lines 135-147
> > 
> >
> > This suggests a change of semantics to `Resources`.
> > 
> > Currently: `Resources` always refers to a valid collection resources. 
> > This function would never return Some(Error) unless we change the semantics.
> > 
> > If we want to remove the unnecessary validation, we can do this by 
> > making '`operator += (const Resources& that)`' avoid validating '`that`' 
> > since it's already valid. Currently it just calls '`operator += (const 
> > Resource& r)`' which is unaware of '`r`' already being valid since it comes 
> > from a '`Resources`'.
> > 
> > However, we have to keep the validation in '`operator += (const 
> > Resource& r)`', since '`r`' is just a protobuf coming from an arbitrary 
> > caller, it may be invalid and we need to validate.
> > 
> > Make sense?
> 
> Guangya Liu wrote:
> Hi Ben, does there are any example in mesos code for `Resource` object 
> come from protobuf? I found that we are always using `Resource::parse` to get 
> a `Resource` or `Resources` object and I have added some validation logic in 
> `Resource::parse` in when getting `Resource` or `Resources` object.
> 
> Guangya Liu wrote:
> Ben, got your point. We can set `Resource` object as following:
> 
> ```javascript 
> Resource scalar;
> scalar.set_name("cpus");
> scalar.set_type(Value::SCALAR);
> scalar.mutable_scalar()->set_value(1.234);
> ```
> 
> So if we igore the validation for `'operator += (const Resource& r)'`, 
> there might be some erros if the resource was not constructed correctly. This 
> is not a good way to create `Resource` object but we cannot guard the usage 
> for this now.
> 
> I found that the API `'operator += (const Resource& r)'` was not called 
> directly too frequently, but we are using this API `'operator += (const 
> Resources& that)'` for the most of the time, so seems it is good to keep the 
> validation for `'operator += (const Resource& r)'` as it was not called 
> directly too much. 
> 
> The problem is that `'operator += (const Resources& that)'` is calling 
> `'operator += (const Resource& r)'`, I did not found a good way to keep 
> validation for `'operator += (const Resource& r)'` but ignore the validation 
> for `'operator += (const Resources& that)'`, still checking how we can make 
> this work, any comments/suggestions for this?
> 
> Guangya Liu wrote:
> Did more test by removing validation, the performance of resources 
> operations with `port` resources increased more than 5x.
> 
> With validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 11.212199secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 8.145903secs to perform 1000 'total -= r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 11.154993secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 7.974004secs to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (38490 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (38490 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (38512 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> Without validation
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 2.90743secs to perform 1000 'total += r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9166us to perform 1000 'total -= r' operations on ports(*):[1-2, 
> 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 3.038083secs to perform 1000 'total = total + r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> Took 9253us to perform 1000 'total = total - r' operations on 
> ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 
> (5966 ms)
> [--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test 
> (5966 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (5988 ms total)
> [  PASSED  ] 1 test.
> ```

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Guangya Liu]

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

(Updated 七月 18, 2016, 10:13 a.m.)


Review request for mesos, Benjamin Mahler and Klaus Ma.


Repository: mesos


Description
---

The "validation" API was called in a huge number of times, but this
was only needed when parsing resources and we do not need to do
other validation when doing resources operations, such as add etc.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing
---

The `qcachegrind` result here: 
https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing

After the fix, the performance of `adding` resources increased 30+% when adding 
5 agents to the cluster.

Before fix:
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30
Using 5 agents and 1 clients
Added 1 clients in 47us
**Added 5 agents in 1.312497secs**
Sorted 1 clients in 43us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 (1321 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31
Using 5 agents and 50 clients
Added 50 clients in 948us
**Added 5 agents in 1.325987secs**
Sorted 50 clients in 1165us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 (1340 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32
Using 5 agents and 100 clients
Added 100 clients in 1697us
**Added 5 agents in 1.409478secs**
Sorted 100 clients in 2876us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 (1432 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33
Using 5 agents and 200 clients
Added 200 clients in 4553us
**Added 5 agents in 1.371473secs**
Sorted 200 clients in 5371us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33 (1412 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34
Using 5 agents and 500 clients
Added 500 clients in 8836us
**Added 5 agents in 1.304245secs**
Sorted 500 clients in 14697us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34 (1387 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
Using 5 agents and 1000 clients
Added 1000 clients in 19508us
**Added 5 agents in 1.270555secs**
Sorted 1000 clients in 32575us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (1433 ms)


After the fix:
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30
Using 5 agents and 1 clients
Added 1 clients in 42us
**Added 5 agents in 891266us**
Sorted 1 clients in 59us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 (902 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31
Using 5 agents and 50 clients
Added 50 clients in 933us
**Added 5 agents in 885006us**
Sorted 50 clients in 1220us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 (899 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32
Using 5 agents and 100 clients
Added 100 clients in 1879us
**Added 5 agents in 903112us**
Sorted 100 clients in 2800us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 (922 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33
Using 5 agents and 200 clients
Added 200 clients in 3893us
**Added 5 agents in 881240us**
Sorted 200 clients in 5802us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33 (912 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34
Using 5 agents and 500 clients
Added 500 clients in 10712us
**Added 5 agents in 877442us**
Sorted 500 clients in 17887us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34 (949 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
Using 5 agents and 1000 clients
Added 1000 clients in 21472us
**Added 5 agents in 916653us**
Sorted 1000 clients in 37369us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (1057 ms)


Thanks,

Guangya Liu

Review Request 50128: Added Parse() and Serialize() to Docker::Device.

[Yubo Li]

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

Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

Wrapped Parse() and Serialize() functions to Docker::Device structure.


Diffs
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 515842d381ca8a91ad481f66c7be057dff2f3f28 
  src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 
  src/slave/containerizer/docker.cpp f1ecf3b25d85597f6c3dcaa47968860ed119dbd5 

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


Testing
---

make check


Thanks,

Yubo Li

Review Request 50126: Fixed tests interfaces for GPU docker containerizer.

[Yubo Li]

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

Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

This fixed API changes of testing module against docker containerizer
API changes to support GPU.


Diffs
-

  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/mesos.cpp d073d79c5797ecb021f0294ab6586a000f3ca600 

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


Testing
---

make check


Thanks,

Yubo Li

Review Request 50127: Added NvidiaGpuDockerContainerizerTest.ROOT_DOCKER_LaunchWithGpu.

[Yubo Li]

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

Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

This added a testing case for end-to-end GPU support for docker
containerizer.


Diffs
-

  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
7944c0da716b2438fa2b40f5504aefa346e31046 

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


Testing
---

GTEST_FILTER="NvidiaGpuDockerContainerizerTest.ROOT_DOCKER_LaunchWithGpu" make 
-j check


Thanks,

Yubo Li

Review Request 50125: Added mesos-docker-executor support for '--device'.

[Yubo Li]

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

Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

This gave mesos-docker-executor capability to parse '--device' flag,
re-arrange device information format, and pass to 'docker::run()' for
device exposition.


Diffs
-

  src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 

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


Testing
---

make check


Thanks,

Yubo Li

Review Request 50124: Added '--device' flag to mesos-docker-executor.

[Yubo Li]

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

Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

Added a new flag '--device' to mesos-docker-executor to point out the
devices can be accessed in docker container. Use the format
PathInHost:PathInContainer:Permission and use comma to split multiple
devices.


Diffs
-

  src/docker/executor.hpp 7b63d784d6b8685912598b77fb38cf6e70646ae3 
  src/slave/containerizer/docker.cpp f1ecf3b25d85597f6c3dcaa47968860ed119dbd5 

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


Testing
---

make check


Thanks,

Yubo Li

Review Request 50123: Added GPU scheduler for docker containerizer.

[Yubo Li]

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

Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

This added 'NvidiaGpuAllocator' to docker containerizer so that the
docker containerizer can use it to allocate GPUs to the task with 'gpus'
resource. Also, allocated GPUs will automatically deallocated after the
job destroyed.


Diffs
-

  src/slave/containerizer/docker.hpp 43ca4317d608b3b43dd7bd0d1b55c721e7364885 
  src/slave/containerizer/docker.cpp f1ecf3b25d85597f6c3dcaa47968860ed119dbd5 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 49914: Improved the speed of 'MasterAPITest.UnreserveResources'.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49914, 50072, 49913]

Failed command: ./support/apply-review.sh -n -r 50072

Error:
2016-07-18 09:08:34 URL:https://reviews.apache.org/r/50072/diff/raw/ 
[1099/1099] -> "50072.patch" [1]
Total errors found: 0
Checking 1 files
Error: No line in the commit message summary may exceed 72 characters.

Full log: https://builds.apache.org/job/mesos-reviewbot/14370/console

- Mesos ReviewBot


On July 18, 2016, 8:20 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49914/
> ---
> 
> (Updated July 18, 2016, 8:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-5732
> https://issues.apache.org/jira/browse/MESOS-5732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this patch, speed of 'MasterAPITest.UnreserveResources'
> is increased by removing the unnecessary check for unreserved
> resources which is already verified in
> 'MasterAPITest.ReserveResources' and thus by removing the
> delay for waiting for the resource offers to be declined.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 
> 
> Diff: https://reviews.apache.org/r/49914/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50019: Ensured that framework IDs are quoted consistency in log messages.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On July 14, 2016, 7:58 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50019/
> ---
> 
> (Updated July 14, 2016, 7:58 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework IDs are assigned by the master, so we know they follow a
> predictable format that does not contain whitespace. Hence, when
> including framework IDs in log messages, we don't need to quote
> them. Most places in the codebase didn't use quotes, but a few places
> did; this commit removes the single quotes from the latter.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ee26f02cbac0cef5e77b536a5842e5ea4bf27ea2 
>   src/examples/test_http_framework.cpp 
> cf6baed07c862ccade080618f33cc921fc09415d 
>   src/master/http.cpp d1fe22bd2d9eb6bd82e32cf01720bbe3d8be26d5 
>   src/sched/sched.cpp b9315b53dfbe86500ad1643cc7dfd0f9d6f6a32a 
>   src/slave/containerizer/docker.cpp f1ecf3b25d85597f6c3dcaa47968860ed119dbd5 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e900d752b70165236a198f70c8cb24876a678e4b 
> 
> Diff: https://reviews.apache.org/r/50019/diff/
> 
> 
> Testing
> ---
> 
> make check, visual inspection
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49914: Improved the speed of 'MasterAPITest.UnreserveResources'.

[Abhishek Dasgupta]

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

(Updated July 18, 2016, 8:20 a.m.)


Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

In this patch, speed of 'MasterAPITest.UnreserveResources'
is increased by removing the unnecessary check for unreserved
resources which is already verified in
'MasterAPITest.ReserveResources' and thus by removing the
delay for waiting for the resource offers to be declined.


Diffs
-

  src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 

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


Testing
---

On Ubuntu 16.04:
sudo make -j4 check


Thanks,

Abhishek Dasgupta

Re: Review Request 49914: Improved the speed of 'MasterAPITest.UnreserveResources'.

[Abhishek Dasgupta]

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

(Updated July 18, 2016, 8:19 a.m.)


Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Improved the speed of 'MasterAPITest.UnreserveResources'.


Diffs (updated)
-

  src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 

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


Testing
---

On Ubuntu 16.04:
sudo make -j4 check


Thanks,

Abhishek Dasgupta

Re: Review Request 50072: Set allocation interval in 'Master Flags' for operator API reservation tests.

[Abhishek Dasgupta]

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

(Updated July 18, 2016, 8:16 a.m.)


Review request for mesos, Anand Mazumdar and Neil Conway.


Summary (updated)
-

Set allocation interval in 'Master Flags' for operator API reservation tests.


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


Repository: mesos


Description (updated)
---

Set allocation interval in 'Master Flags' for operator
API reservation tests.


Diffs (updated)
-

  src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49913: Moved createFrameworkInfo() function definition to tests/mesos.hpp.

[Abhishek Dasgupta]

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

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


Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.


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


Repository: mesos


Description
---

There were multiple createFrameworkInfo() definition scattered
in various testcases. In this patch, only one defintion of
createFrameworkInfo() is provided in src/tests/mesos.hpp and
other tests just used this definition.


Diffs (updated)
-

  src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/persistent_volume_endpoints_tests.cpp 
2a22f3b0da817e650a25e5e2c951adb7462718b4 
  src/tests/reservation_endpoints_tests.cpp 
48c002d1dc371c285b9421ef5a2c57250d270fa8 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 50117: Added more elapse time for allocator benchmark test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49943, 49781, 50060, 50061, 50062, 50115, 50117]

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 18, 2016, 5:29 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50117/
> ---
> 
> (Updated July 18, 2016, 5:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is adding elapse time for addFramework, addSlave
> and setQuota in allocator benchmark test, adding those elapse
> time can help evaluate the performnace of those APIs.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
> 
> Diff: https://reviews.apache.org/r/50117/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> DeclineOffers benchmark
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 14779us
> Added 1000 agents in 2.7833secs
> round 0 allocate() took 2.325974secs to make 1000 offers
> round 1 allocate() took 2.349536secs to make 1000 offers
> round 2 allocate() took 2.368871secs to make 1000 offers
> round 3 allocate() took 2.584638secs to make 1000 offers
> ```
> 
> ResourceLabels benchmark
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 12574us
> Added 1000 agents in 3.37232secs
> round 0 allocate() took 2.459538secs to make 1000 offers
> round 1 allocate() took 2.474587secs to make 1000 offers
> ```
> 
> Mtrics benchmark
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/1
> Set quota for 50 roles in 64060us
> Added 50 frameworks in 97191us
> Added 1000 agents in 9.80secs
> /metrics/snapshot took 78252us for 1000 agents and 50 frameworks
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.Metrics/1 (10055 
> ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (10055 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (10079 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>