Review Request 70591: Introduced struct RoleInfo to track reservations and framework IDs in one map.

2019-05-03 Thread Andrei Sekretenko 

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This patch introduces `struct RoleInfo` which contains the framework IDs and 
reservations tied to the role and replaces `roles` and 
`reservationScalarQuantities` hashmaps with `hashmap`.

I personally do not like the name `RoleInfo` and would appreciate any better 
naming ideas. 
However, naming this entity `Role` would have been even worse: it would imply 
using `const Role& role` throughout the code, which would require changing all 
the places which use `std::string role` as a key...


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
  src/master/allocator/mesos/hierarchical.cpp 
64a076ddd29711437d539a06bb0470755828cc87 


Diff: https://reviews.apache.org/r/70591/diff/1/


Testing
---

make check

Benchmarking: 5 runs of 
`BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5` with the 
optimized build.

Performance impact in this benchmark seems to be negligible.

BEFORE:
Added 3000 agents in 51.553929ms
Added 3000 frameworks in 15.174748344secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.400805171secs
Made 0 allocation in 12.5850238secs

Added 3000 agents in 55.739336ms
Added 3000 frameworks in 14.730404769secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.563439682secs
Made 0 allocation in 13.063555055secs

Added 3000 agents in 54.414733ms
Added 3000 frameworks in 15.10136842secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 13.501664915secs
Made 0 allocation in 12.89034382secs

Added 3000 agents in 52.58252ms
Added 3000 frameworks in 14.048350298secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.299952145secs
Made 0 allocation in 11.888248811secs

Added 3000 agents in 52.821439ms
Added 3000 frameworks in 15.344450583secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.63425secs
Made 0 allocation in 12.427171541secs


AFTER:

Added 3000 agents in 69.716648ms
Added 3000 frameworks in 15.249001979secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.860494226secs
Made 0 allocation in 12.228866329secs

Added 3000 agents in 52.639388ms
Added 3000 frameworks in 15.207895482secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.504777266secs
Made 0 allocation in 12.70388062secs

Added 3000 agents in 56.865794ms
Added 3000 frameworks in 15.284003915secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 13.86859815secs
Made 0 allocation in 12.538958231secs

Added 3000 agents in 56.028013ms
Added 3000 frameworks in 13.892577869secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.341724418secs
Made 0 allocation in 12.23022189secs

Added 3000 agents in 52.368219ms
Added 3000 frameworks in 13.978581104secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
Made 3856 allocations in 12.701682501secs
Made 0 allocation in 12.141360313secs


Thanks,

Andrei Sekretenko

Re: Review Request 70591: Introduced struct RoleInfo to track reservations and framework IDs in one map.

2019-05-04 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [70591]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 3, 2019, 3:50 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70591/
> ---
> 
> (Updated May 3, 2019, 3:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9701
> https://issues.apache.org/jira/browse/MESOS-9701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `struct RoleInfo` which contains the framework IDs and 
> reservations tied to the role and replaces `roles` and 
> `reservationScalarQuantities` hashmaps with `hashmap`.
> 
> I personally do not like the name `RoleInfo` and would appreciate any better 
> naming ideas. 
> However, naming this entity `Role` would have been even worse: it would imply 
> using `const Role& role` throughout the code, which would require changing 
> all the places which use `std::string role` as a key...
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
>   src/master/allocator/mesos/hierarchical.cpp 
> 64a076ddd29711437d539a06bb0470755828cc87 
> 
> 
> Diff: https://reviews.apache.org/r/70591/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarking: 5 runs of 
> `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5` with 
> the optimized build.
> 
> Performance impact in this benchmark seems to be negligible.
> 
> BEFORE:
> Added 3000 agents in 51.553929ms
> Added 3000 frameworks in 15.174748344secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.400805171secs
> Made 0 allocation in 12.5850238secs
> 
> Added 3000 agents in 55.739336ms
> Added 3000 frameworks in 14.730404769secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.563439682secs
> Made 0 allocation in 13.063555055secs
> 
> Added 3000 agents in 54.414733ms
> Added 3000 frameworks in 15.10136842secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.501664915secs
> Made 0 allocation in 12.89034382secs
> 
> Added 3000 agents in 52.58252ms
> Added 3000 frameworks in 14.048350298secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.299952145secs
> Made 0 allocation in 11.888248811secs
> 
> Added 3000 agents in 52.821439ms
> Added 3000 frameworks in 15.344450583secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.63425secs
> Made 0 allocation in 12.427171541secs
> 
> 
> AFTER:
> 
> Added 3000 agents in 69.716648ms
> Added 3000 frameworks in 15.249001979secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.860494226secs
> Made 0 allocation in 12.228866329secs
> 
> Added 3000 agents in 52.639388ms
> Added 3000 frameworks in 15.207895482secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.504777266secs
> Made 0 allocation in 12.70388062secs
> 
> Added 3000 agents in 56.865794ms
> Added 3000 frameworks in 15.284003915secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.86859815secs
> Made 0 allocation in 12.538958231secs
> 
> Added 3000 agents in 56.028013ms
> Added 3000 frameworks in 13.892577869secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.341724418secs
> Made 0 allocation in 12.23022189secs
> 
> Added 3000 agents in 52.368219ms
> Added 3000 frameworks in 13.978581104secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.701682501secs
> Made 0 allocation in 12.141360313secs
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70591: Introduced struct RoleInfo to track reservations and framework IDs in one map.

2019-05-06 Thread Meng Zhu 

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




src/master/allocator/mesos/hierarchical.hpp
Line 542 (original), 555 (patched)


s/.../and when there are reservations tied to this role/



src/master/allocator/mesos/hierarchical.hpp
Line 543 (original), 556 (patched)


Calling it `roles`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1716 (patched)


newline below



src/master/allocator/mesos/hierarchical.cpp
Line 2119 (original), 2122 (patched)


Shouldn't this be:

```
if (!roles.contains(role) || roles.at(role).frameworks.empty()) {
  
}
```



src/master/allocator/mesos/hierarchical.cpp
Line 2610 (original), 2614 (patched)


not yours, but I found this shadowing hurting readibility. Maybe we can 
sneak this fix in?


- Meng Zhu


On May 3, 2019, 8:50 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70591/
> ---
> 
> (Updated May 3, 2019, 8:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9701
> https://issues.apache.org/jira/browse/MESOS-9701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `struct RoleInfo` which contains the framework IDs and 
> reservations tied to the role and replaces `roles` and 
> `reservationScalarQuantities` hashmaps with `hashmap`.
> 
> I personally do not like the name `RoleInfo` and would appreciate any better 
> naming ideas. 
> However, naming this entity `Role` would have been even worse: it would imply 
> using `const Role& role` throughout the code, which would require changing 
> all the places which use `std::string role` as a key...
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
>   src/master/allocator/mesos/hierarchical.cpp 
> 64a076ddd29711437d539a06bb0470755828cc87 
> 
> 
> Diff: https://reviews.apache.org/r/70591/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarking: 5 runs of 
> `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5` with 
> the optimized build.
> 
> Performance impact in this benchmark seems to be negligible.
> 
> BEFORE:
> Added 3000 agents in 51.553929ms
> Added 3000 frameworks in 15.174748344secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.400805171secs
> Made 0 allocation in 12.5850238secs
> 
> Added 3000 agents in 55.739336ms
> Added 3000 frameworks in 14.730404769secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.563439682secs
> Made 0 allocation in 13.063555055secs
> 
> Added 3000 agents in 54.414733ms
> Added 3000 frameworks in 15.10136842secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.501664915secs
> Made 0 allocation in 12.89034382secs
> 
> Added 3000 agents in 52.58252ms
> Added 3000 frameworks in 14.048350298secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.299952145secs
> Made 0 allocation in 11.888248811secs
> 
> Added 3000 agents in 52.821439ms
> Added 3000 frameworks in 15.344450583secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.63425secs
> Made 0 allocation in 12.427171541secs
> 
> 
> AFTER:
> 
> Added 3000 agents in 69.716648ms
> Added 3000 frameworks in 15.249001979secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.860494226secs
> Made 0 allocation in 12.228866329secs
> 
> Added 3000 agents in 52.639388ms
> Added 3000 frameworks in 15.207895482secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.504777266secs
> Made 0 allocation in 12.70388062secs
> 
> Added 3000 agents in 56.865794ms
> Added 3000 frameworks in 15.284003915secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.86859815secs
> Made 0 allocation in 12.538958231secs
> 
> Added 3000 agents in 56.028013ms
> Added 3000 frameworks in 13.892577869secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.341724418secs
> Made 0 allocation in 12.2

Re: Review Request 70591: Introduced struct RoleInfo to track reservations and framework IDs in one map.

2019-05-06 Thread Meng Zhu 

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



Regarding the commit message. We usually write the commit message more formally 
and leave out the discussion part. The discussion should happen while working 
on the patch and the commit description should only include the discussion 
result/decision. My take of an ideal commit message is (1) describe the change 
(2) (optional) document the current (old) way (3) why the change (simplify, 
perf improvement...) (4) (optional) benchmark result if any.

Also, we try to align the length with the summary (i.e. 80char length). How 
about:

This patch introduces `struct RoleInfo` in the allocator which contains the
framework IDs and reservations tied to a role. A single hashmap using the struct
replaces the two existing separate hashmaps. This simplifies the role tracking
logic in the allocator. The patch introduces minimal to no performance impact.

- Meng Zhu


On May 3, 2019, 8:50 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70591/
> ---
> 
> (Updated May 3, 2019, 8:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9701
> https://issues.apache.org/jira/browse/MESOS-9701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces `struct RoleInfo` which contains the framework IDs and 
> reservations tied to the role and replaces `roles` and 
> `reservationScalarQuantities` hashmaps with `hashmap`.
> 
> I personally do not like the name `RoleInfo` and would appreciate any better 
> naming ideas. 
> However, naming this entity `Role` would have been even worse: it would imply 
> using `const Role& role` throughout the code, which would require changing 
> all the places which use `std::string role` as a key...
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
>   src/master/allocator/mesos/hierarchical.cpp 
> 64a076ddd29711437d539a06bb0470755828cc87 
> 
> 
> Diff: https://reviews.apache.org/r/70591/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarking: 5 runs of 
> `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5` with 
> the optimized build.
> 
> Performance impact in this benchmark seems to be negligible.
> 
> BEFORE:
> Added 3000 agents in 51.553929ms
> Added 3000 frameworks in 15.174748344secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.400805171secs
> Made 0 allocation in 12.5850238secs
> 
> Added 3000 agents in 55.739336ms
> Added 3000 frameworks in 14.730404769secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.563439682secs
> Made 0 allocation in 13.063555055secs
> 
> Added 3000 agents in 54.414733ms
> Added 3000 frameworks in 15.10136842secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.501664915secs
> Made 0 allocation in 12.89034382secs
> 
> Added 3000 agents in 52.58252ms
> Added 3000 frameworks in 14.048350298secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.299952145secs
> Made 0 allocation in 11.888248811secs
> 
> Added 3000 agents in 52.821439ms
> Added 3000 frameworks in 15.344450583secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.63425secs
> Made 0 allocation in 12.427171541secs
> 
> 
> AFTER:
> 
> Added 3000 agents in 69.716648ms
> Added 3000 frameworks in 15.249001979secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.860494226secs
> Made 0 allocation in 12.228866329secs
> 
> Added 3000 agents in 52.639388ms
> Added 3000 frameworks in 15.207895482secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.504777266secs
> Made 0 allocation in 12.70388062secs
> 
> Added 3000 agents in 56.865794ms
> Added 3000 frameworks in 15.284003915secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.86859815secs
> Made 0 allocation in 12.538958231secs
> 
> Added 3000 agents in 56.028013ms
> Added 3000 frameworks in 13.892577869secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.341724418secs
> Made 0 allocation in 12.23022189secs
> 
> Added 3000 agents in 52.368219ms
> Added 3000 frameworks in 13.978581104secs
> Benchmark setup: 3000 agents, 3000 roles, 3