mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 52103: Implement quota update through `PUT` method.
Date Thu, 20 Oct 2016 15:53:41 GMT

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



What about updating "quota.md"? How a quota update request will look like for an operator?
I assume the same as quota set, but we should call it out in the docs.


src/master/allocator/mesos/hierarchical.cpp (lines 1140 - 1141)
<https://reviews.apache.org/r/52103/#comment222695>

    Even if we leave one function in the allocator for setting *and* updating quota (I'm not
convinced yet) we should, don't you think it is valuable to leave most of this comment so
people understand why there are two separate paths?



src/master/allocator/mesos/hierarchical.cpp (lines 1205 - 1207)
<https://reviews.apache.org/r/52103/#comment222694>

    Ups.



src/master/http.cpp (line 2349)
<https://reviews.apache.org/r/52103/#comment222690>

    s/for the first time/without a quota/



src/master/master.hpp (lines 1028 - 1032)
<https://reviews.apache.org/r/52103/#comment222699>

    We definitely need an override taking a master call, hence we should update the operator
API as well. Feel free to do it either here or as a separate commit. You can look into https://reviews.apache.org/r/49247
for inspiration.



src/master/quota_handler.cpp (line 81)
<https://reviews.apache.org/r/52103/#comment222701>

    This should  go away, either altogether or replaced by another comment.



src/master/quota_handler.cpp (lines 82 - 87)
<https://reviews.apache.org/r/52103/#comment222700>

    Instead, you can do a check just for the delta (new - old), and only if the delta is positive.



src/master/quota_handler.cpp (line 363)
<https://reviews.apache.org/r/52103/#comment222707>

    This should go away if you decide to keep both `set()` and `update()`.



src/master/quota_handler.cpp (lines 559 - 562)
<https://reviews.apache.org/r/52103/#comment222711>

    This function is almost identical to `_set()`. Have you explored the possibility to calculate
the delta (new - old) and re-use the set path if the delta > 0 and remove path otherwise?
    
    If we can't express update internally in terms of set and remove, we should better maintain
a separate update path (including allocator) for simplicity.



src/master/quota_handler.cpp (lines 580 - 585)
<https://reviews.apache.org/r/52103/#comment222705>

    Do we need this? Isn't it subsumed by the next check?



src/master/quota_handler.cpp (line 603)
<https://reviews.apache.org/r/52103/#comment222704>

    In this case you may rescind way more resources than necessary, right? Actaully, if new
< old, we can skip capasity heuristic and rescinding altogether.


- Alexander Rukletsov


On Oct. 19, 2016, 11:28 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52103/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2016, 11:28 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
> 
> 
> Bugs: MESOS-4941
>     https://issues.apache.org/jira/browse/MESOS-4941
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implement quota update through `PUT` method.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 2d56bd011f2c87c67a02d0ae467a4a537d36867e

>   src/master/http.cpp 9005e7c308d5f57c6f5c573951d468a3ba730740 
>   src/master/master.hpp 35db198748b8652eb53e17f592f6b40d1e6a3ed9 
>   src/master/quota_handler.cpp bf6a613a7bb3c62fd77d1ffde3170749d6c21fa2 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
> 
> Diff: https://reviews.apache.org/r/52103/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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