mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 71068: Added offer rescind logic for limits enforcement.
Date Sat, 13 Jul 2019 00:02:41 GMT

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



I'll be sending out a patch soon for the fixing of the role tracking, but it seems that this
patch doesn't care about MESOS-9888/MESOS-9890 since the non-default quota case will always
have an entry in `roles`, right?

Looks good, mainly I was (still am) puzzled about what `offeredLimits` is and I think we can
capture the goal condition more directly, see suggestion below


src/master/quota_handler.cpp
Lines 582-586 (patched)
<https://reviews.apache.org/r/71068/#comment303782>

    Do we need another TODO here? This is only a partial rescind since it only considers guarantees,
right? One would expect the rescinding to also take guarantees into account.



src/master/quota_handler.cpp
Lines 594-596 (patched)
<https://reviews.apache.org/r/71068/#comment303785>

    Is this case possible? If so, how? E.g. setting back to default quota?



src/master/quota_handler.cpp
Lines 598-599 (patched)
<https://reviews.apache.org/r/71068/#comment303795>

    Hm.. I had a really hard time figuring out what this variable is storing, and how it works
with the contains check below.
    
    I would expect something like:
    
    ```
    if (limits.contains(consumedQuota + offered)) {
      // done
    }
    ```
    
    i.e. we don't want consumed + offered to exceed the limits
    
    I guess if you work out the arithmetic, it's the same (?) as what the code here is doing?
But, I can't seem to reason about the code here whereas the condition above seems to directly
capture the goal of the rescinding logic?



src/master/quota_handler.cpp
Lines 600 (patched)
<https://reviews.apache.org/r/71068/#comment303791>

    s/currentOffered/offered/ ?



src/master/quota_handler.cpp
Lines 634-636 (patched)
<https://reviews.apache.org/r/71068/#comment303789>

    Hm.. seems more consistent at the top of the loop like the other break in the outer loop?


- Benjamin Mahler


On July 12, 2019, 10:02 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71068/
> -----------------------------------------------------------
> 
> (Updated July 12, 2019, 10:02 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9812
>     https://issues.apache.org/jira/browse/MESOS-9812
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Outstanding offers of a role (including offers allocated to its
> subroles) are rescinded until the sum of its consumed quota and
> outstanding offered resources are below the requested limits.
> 
> The rescind logic here, however, overlooks the race between master
> and the allocator. Namely, there might be pending offers in the master
> mailbox that have yet to be processed. Here, for simplicity, we ignore
> this race. To properly handle this, the plan is to move offer
> management logic into the allocator and rescind offers there.
> 
> Also added a test.
> 
> 
> Diffs
> -----
> 
>   src/master/quota_handler.cpp 13789d8461fcf029837b15b567ac1b7b97677607 
>   src/tests/master_quota_tests.cpp a1b37693b74778ff6827c9325f1f9225ecf73e58 
> 
> 
> Diff: https://reviews.apache.org/r/71068/diff/1/
> 
> 
> Testing
> -------
> 
> Ran continously without failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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