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

2016-07-22 Thread Benjamin Mahler 

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


Ship it!




The commit summary and description seem to not describe the intent of this 
patch, which was to fix the issue of allocating ports outside of the agent's 
port range. E.g. commit message:


Fixed an issue with port allocation in the sorter benchmarks.

Previously the port allocation was not aware of the port range
that the agent had available.

This updates the allocation of fragmented port ranges to fragment
*from* the range of ports available on the agent.
```


src/tests/sorter_tests.cpp (lines 562 - 566)


Let's put this example up in the documentation for this function? Then the 
fragmentation technique is clear to the caller and anyone reading the 
documentation can figure it out.



src/tests/sorter_tests.cpp (lines 567 - 581)


Maybe the following is simpler?

```
  for (size_t i = 0; i < numRanges; ++i) {
Value::Range* range = ranges.add_range();

range->set_begin(bounds.begin() + (i * 2));
range->set_end(range->begin());
  }

  // Make sure the last range covers the end of the bounds.
  if (!ranges.range().empty()) {
ranges.mutable_range()->rbegin()->set_end(bounds.end());
  }
```



src/tests/sorter_tests.cpp (line 570)


Why start instead of begin?



src/tests/sorter_tests.cpp (line 587)


We should probably do a s/make/create/ to be consistent with the similar 
helpers that we had looked at in your other review?


- Benjamin Mahler


On July 21, 2016, 1:43 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50172/
> ---
> 
> (Updated July 21, 2016, 1:43 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added three new functions to sorter benchmark test to
> generate a \`ports\` resource which falls into a specifed ports
> range bounds.
> 
> 1) makePorts: Creates a "ports(*)" resource for the given ranges.
> 2) fragment: Fragments the given range bounds into a number of
>subranges.
> 3) makeRange: Returns the 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 50172: Made `ports` resource configurable in sorter benchmark test.

2016-07-19 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [50172]

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 19, 2016, 3:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50172/
> ---
> 
> (Updated July 19, 2016, 3:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added three new functions to sorter benchmark test to
> generate a `ports` resource which falls into a specifed ports
> range bounds.
> 
> 1) makePorts: Creates a "ports(*)" resource for the given ranges.
> 2) fragment: Fragments the given range bounds into a number of
>subranges.
> 3) makeRange: Returns the 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 50172: Made `ports` resource configurable in sorter benchmark test.

2016-07-19 Thread Guangya Liu 

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

(Updated 七月 19, 2016, 3:59 p.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

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

1) makePorts: Creates a "ports(*)" resource for the given ranges.
2) fragment: Fragments the given range bounds into a number of
   subranges.
3) makeRange: Returns the 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 50172: Made `ports` resource configurable in sorter benchmark test.

2016-07-19 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [50172]

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 19, 2016, 3:35 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, 3:35 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
> [ 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 50172: Made `ports` resource configurable in sorter benchmark test.

2016-07-18 Thread 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 50172: Made `ports` resource configurable in sorter benchmark test.

2016-07-18 Thread 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