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 39285: Added Quota Request Validation.
Date Mon, 02 Nov 2015 16:43:39 GMT

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

Ship it!


Looks good! The only thing I'm not sure about is how to group related checks in the best way.
Do you think it makes sense to create one scope per group and attach related comment to the
scope?


src/master/quota_handler.cpp (line 75)
<https://reviews.apache.org/r/39285/#comment162980>

    Mind adding a comment this is a "reference" role which is deduced from the resources in
the request and is not specified directly? For example:
    
    ```
    // A role for which quota request is sent. Currently only one role per request is allowed.
Since an operator does not specify role explicitly, it is deduced from the provided resources.
    ```



src/master/quota_handler.cpp (line 121)
<https://reviews.apache.org/r/39285/#comment162977>

    s/including/may not include
    
    for consistency with earlier messages



src/master/quota_handler.cpp (line 125)
<https://reviews.apache.org/r/39285/#comment162975>

    Typo: known



src/master/quota_handler.cpp (line 133)
<https://reviews.apache.org/r/39285/#comment162974>

    Backtick `QuotaInfo`?



src/master/quota_handler.cpp (lines 135 - 136)
<https://reviews.apache.org/r/39285/#comment162973>

    As per @joris' comment in the previous review, should we check for errors after `.get()`
calls?



src/master/quota_handler.cpp (lines 157 - 158)
<https://reviews.apache.org/r/39285/#comment162972>

    I think it makes sense not only to mention validation has failed, but also specify where
we are coming from into validate (set quota).
    
    In that vein, I suggest to rephrase as follows:
    "Failed to validate set quota request: " + `quotaInfo.error()`, both for response and
for logging.


- Alexander Rukletsov


On Oct. 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3199
>     https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>


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