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 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 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 50062: Updated makePortRanges for variable types.

[Mesos ReviewBot]

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



Patch looks great!

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

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 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 50062: Updated makePortRanges for variable types.

[Guangya Liu]

---
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 (updated)
---

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