Re: Review Request 70419: Refactor `Sorter::sorted()` to return a stream of clients.

2019-05-01 Thread Benjamin Mahler 

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



Looks good, just hoping we can make the call sites in the allocator a bit more 
readable


src/master/allocator/mesos/hierarchical.cpp
Line 1826 (original), 1827-1831 (patched)


Yikes.. this is a lot less readable vs the foreach

Some suggestions:

```
Sorter::SortedClients sortedRoles = quotaRoleSorter->sort();
auto role = sortedRoles.next();

for (; role.isSome(); role = sortedRoles.next()) {
  CHECK(quotaGuarantees.contains(*role));
  ...
```

Can't we use role directly with the `*` operator?

Or:

```
Sorter::SortedClients sortedRoles = quotaRoleSorter->sort();

while (sortedRoles.hasNext()) {
  const string& role = sortedRoles.next();
  
  CHECK(quotaGuarantees.contains(*role));
  ...
```

Ditto for framework sorting



src/master/allocator/sorter/drf/sorter.hpp
Lines 436-438 (patched)


I think this could be a bit more succinct:

```
// TODO(mzhu): Implement incremental sorting by lazily walking the tree.
```



src/master/allocator/sorter/sorter.hpp
Lines 157 (patched)


"complete" doesn't seem to add anything here? Do you mean "forward"?

It seems a lot more straightforward than I had guessed:


https://stackoverflow.com/questions/27604201/implement-lazy-generator-as-forward-iterator-in-c



src/master/allocator/sorter/sorter.hpp
Lines 172 (patched)


FWICT, SortedClients does not support copy construction or copy assignment 
since it stores a unique_ptr, and the code compiles today because all the call 
sides are doing move construction.

Let's be explicit about what we support and don't support? Something like 
this?

```
virtual SortedClients(SortedClients&&) = default;
virtual SortedClients& operator=(SortedClients&&) = default;

virtual SortedClients(const SortedClients&) = delete;
virtual SortedClients& operator=(const SortedClients&) = delete;
```

Ditto for the impl?



src/tests/sorter_tests.cpp
Lines 50 (patched)


Seems like the right thing here is to take an rvalue reference rather than 
copying?

Doesn't matter much for the test but it also just makes the intent clearer: 
this will "consume" the sorted clients by iterating over it, and produce a 
vector result.

Actually, I'm puzzled at how copying here even works. I suspect what's 
happening is that *all* call sites are passing `SortedClients&&` and therefore 
the move constructor `SortedClients(SortedClients&&)` is getting invoked and no 
copying occurs. If a call site pass a non rvalue, I don't see how it will 
compile. All the more reason to take an rvalue here and probably explictly 
disable copying for now, since that requires us to write the copying code 
manually to get it correct.



src/tests/sorter_tests.cpp
Lines 62 (patched)


add another newline


- Benjamin Mahler


On May 1, 2019, 1:17 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70419/
> ---
> 
> (Updated May 1, 2019, 1:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9722
> https://issues.apache.org/jira/browse/MESOS-9722
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch refactors `Sorter::sorted` to return
> `class SortedClients` instead of a whole vector.
> Callers can then use SortedClients::next() to get the
> next sorted client. This paves the way for sort optimization
> where sorting of the whole clients can be lazily done as callers
> ask for the next client.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 64a076ddd29711437d539a06bb0470755828cc87 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 91a9d668b87079158f7072780dc86bb08865166e 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 9367469132e426f0b4b66a80ad300c157fba6bf2 
>   src/master/allocator/sorter/random/sorter.hpp 
> 55e22d7705f163fe47d5aa47416ee0714c5a87c0 
>   src/master/allocator/sorter/random/sorter.cpp 
> 813f5b55d38dd9fa822de53ee944c3f72251a69d 
>   src/master/allocator/sorter/sorter.hpp 
> d56a1166a9e82b034564842ac071874ec2885004 
>   src/tests/sorter_tests.cpp 9aee2b41b0d3c978bca6bd2d7ad28e32506a648a 
> 
> 
>

Re: Review Request 70419: Refactor `Sorter::sorted()` to return a stream of clients.

2019-04-30 Thread Meng Zhu 

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

(Updated April 30, 2019, 6:17 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Used PIMPL to return `SortedClients` by value.


Summary (updated)
-

Refactor `Sorter::sorted()` to return a stream of clients.


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


Repository: mesos


Description (updated)
---

This patch refactors `Sorter::sorted` to return
`class SortedClients` instead of a whole vector.
Callers can then use SortedClients::next() to get the
next sorted client. This paves the way for sort optimization
where sorting of the whole clients can be lazily done as callers
ask for the next client.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
64a076ddd29711437d539a06bb0470755828cc87 
  src/master/allocator/sorter/drf/sorter.hpp 
91a9d668b87079158f7072780dc86bb08865166e 
  src/master/allocator/sorter/drf/sorter.cpp 
9367469132e426f0b4b66a80ad300c157fba6bf2 
  src/master/allocator/sorter/random/sorter.hpp 
55e22d7705f163fe47d5aa47416ee0714c5a87c0 
  src/master/allocator/sorter/random/sorter.cpp 
813f5b55d38dd9fa822de53ee944c3f72251a69d 
  src/master/allocator/sorter/sorter.hpp 
d56a1166a9e82b034564842ac071874ec2885004 
  src/tests/sorter_tests.cpp 9aee2b41b0d3c978bca6bd2d7ad28e32506a648a 


Diff: https://reviews.apache.org/r/70419/diff/4/

Changes: https://reviews.apache.org/r/70419/diff/3-4/


Testing (updated)
---

make check

See benchmark result in https://reviews.apache.org/r/70497/


Thanks,

Meng Zhu