mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 70618: Store framework sorters inside RoleInfos.
Date Mon, 20 May 2019 12:18:35 GMT

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



Just a partial review, will post the rest later


src/master/allocator/mesos/hierarchical.hpp
Line 110 (original), 111 (patched)
<https://reviews.apache.org/r/70618/#comment302016>

    space before brace
    
    Did I mention this in the last patch? Did you rebase?



src/master/allocator/mesos/hierarchical.hpp
Lines 112 (patched)
<https://reviews.apache.org/r/70618/#comment302017>

    You have a user-defined constructor declared below. So this is not strictly needed. And
(correct me if I am wrong) I do not think our style guide requires this, I would suggest removing
this for brevity.



src/master/allocator/mesos/hierarchical.hpp
Lines 115 (patched)
<https://reviews.apache.org/r/70618/#comment302018>

    Put this in the line above.



src/master/allocator/mesos/hierarchical.hpp
Lines 118-120 (original), 128-133 (patched)
<https://reviews.apache.org/r/70618/#comment302019>

    The original comment is verbose and outdated. Let's take this opportunity to improve it.
Also, let's make an effort to align the comment length :)
    
    - Let's remove the ties to DRF (as it can be any sorter algorithm)
    - Let's remove the details of level and stage, readers who are familiar with
    the matter do not need to comment, those who are not will just be confused. Also,
    the structure of the code could change in the future.
    - The comment regarding resource pool is no longer true due to: 
    https://github.com/apache/mesos/commit/a3626219b402ff837192208b6d35946b3a069ce6
    
    How about:
    
    // The sorter determines the resource allocation order of the
    // frameworks subscribed under this role.



src/master/allocator/mesos/hierarchical.cpp
Lines 358-359 (original), 358-359 (patched)
<https://reviews.apache.org/r/70618/#comment302020>

    // FrameworkID may not be present in RoleInfo because the
    // framework was previously deactivated and never re-added.



src/master/allocator/mesos/hierarchical.cpp
Line 367 (original), 367 (patched)
<https://reviews.apache.org/r/70618/#comment302021>

    Not yours, but this could be just a reference?
    
    const hashmap<SlaveID, Resources>& allocation = ...
    
    Want to add a quick patch for that?



src/master/allocator/mesos/hierarchical.cpp
Line 888 (original), 888 (patched)
<https://reviews.apache.org/r/70618/#comment302022>

    hmm, it is unfortunate that we need to make a copy here due to the sorter interface....



src/master/allocator/mesos/hierarchical.cpp
Lines 982-1000 (original), 981-999 (patched)
<https://reviews.apache.org/r/70618/#comment302023>

    We should just use `ResourceQuantities` for these.
    Feel free to do this in another patch.



src/master/allocator/mesos/hierarchical.cpp
Line 1369 (original), 1367 (patched)
<https://reviews.apache.org/r/70618/#comment302024>

    suppressingRoles



src/master/allocator/mesos/hierarchical.cpp
Line 1373 (original), 1372 (patched)
<https://reviews.apache.org/r/70618/#comment302025>

    newline not necessary?



src/master/allocator/mesos/hierarchical.cpp
Line 1395 (original), 1393 (patched)
<https://reviews.apache.org/r/70618/#comment302026>

    revivingRoles



src/master/allocator/mesos/hierarchical.cpp
Line 1402 (original), 1401 (patched)
<https://reviews.apache.org/r/70618/#comment302027>

    newline not necessary?



src/master/allocator/mesos/hierarchical.cpp
Lines 1708-1709 (original), 1706-1709 (patched)
<https://reviews.apache.org/r/70618/#comment302028>

    The logic here is not related to this patch.
    I am OK with either doing this in the previous patch or keep it as is.



src/master/allocator/mesos/hierarchical.cpp
Line 1714 (original), 1714 (patched)
<https://reviews.apache.org/r/70618/#comment302030>

    This should be in the previous patch (I just raised an issue in the patch)



src/master/allocator/mesos/hierarchical.cpp
Line 1853 (original), 1853 (patched)
<https://reviews.apache.org/r/70618/#comment302032>

    Not yours, but this comment is tied to DRF, we should remove/modify it.



src/master/allocator/mesos/hierarchical.cpp
Lines 2169 (patched)
<https://reviews.apache.org/r/70618/#comment302034>

    space before braces



src/master/allocator/mesos/hierarchical.cpp
Lines 2171 (patched)
<https://reviews.apache.org/r/70618/#comment302033>

    newline here


- Meng Zhu


On May 16, 2019, 7:17 a.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70618/
> -----------------------------------------------------------
> 
> (Updated May 16, 2019, 7:17 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 replaces a hashmap<Owned<Sorter>> used for tracking
> framework sorters with a unique_ptr<Sorter> inside of the RoleInfo.
> This simplifies the framework tracking logic and slightly improves
> performance.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp c2058baca5159da4cdcab77afd5de3c0d5ae6c48

>   src/master/allocator/mesos/hierarchical.cpp 875cfcd091f5f58afb89e752da5100a75828dd67

> 
> 
> Diff: https://reviews.apache.org/r/70618/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> Benchmarking: 5 runs of BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5
with the optimized build.
> 
> BEFORE (master):
> 
> Added 3000 agents in 57.444301ms
> Added 3000 frameworks in 15.100499419secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.359926851secs
> Made 0 allocation in 12.147765014secs
> 
> Added 3000 agents in 58.651887ms
> Added 3000 frameworks in 14.485925735secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.6526783secs
> Made 0 allocation in 12.138439924secs
> 
> Added 3000 agents in 59.028581ms
> Added 3000 frameworks in 14.72945866secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.739135078secs
> Made 0 allocation in 12.673302496secs
> 
> Added 3000 agents in 59.050577ms
> Added 3000 frameworks in 14.78273576secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.20400492secs
> Made 0 allocation in 13.289808943secs
> 
> Added 3000 agents in 58.629888ms
> Added 3000 frameworks in 14.714786337secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.721094744secs
> Made 0 allocation in 12.688377237secs
> 
> -----------------------------------
> AFTER (master + previous patch https://reviews.apache.org/r/70591/ + this patch):
> 
> Added 3000 agents in 58.04155ms
> Added 3000 frameworks in 13.694651977secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.417541213secs
> Made 0 allocation in 12.130755905secs
> 
> Added 3000 agents in 55.246813ms
> Added 3000 frameworks in 13.460479936secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.393765514secs
> Made 0 allocation in 12.196981426secs
> 
> Added 3000 agents in 58.013477ms
> Added 3000 frameworks in 13.69361015secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.245916159secs
> Made 0 allocation in 11.699553888secs
> 
> Added 3000 agents in 57.163681ms
> Added 3000 frameworks in 13.738218369secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.809206431secs
> Made 0 allocation in 12.400140164secs
> 
> Added 3000 agents in 57.942087ms
> Added 3000 frameworks in 13.836390727secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.75595625secs
> Made 0 allocation in 11.939263998secs
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message