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



src/master/master.hpp (line 878)
<https://reviews.apache.org/r/39285/#comment161593>

    s/Check/Checks
    s/Request/request
    
    Do we capitalize "quota"?



src/master/master.hpp (lines 879 - 882)
<https://reviews.apache.org/r/39285/#comment161595>

    Let's document more:
    - Role is known to the master.
    - Irrelevant fields in `Resources` are not set (i.e. reservation, disk).



src/master/master.hpp (lines 1976 - 1985)
<https://reviews.apache.org/r/39285/#comment161594>

    Either comment or bookkeeping should go away. As per comment, do you think 
we need `quotaInfo` here now? It does not seem you use this variable in this 
RR. I'd suggest to kill `quotaInfo` here.



src/master/quota_handler.cpp (lines 51 - 52)
<https://reviews.apache.org/r/39285/#comment161604>

    This does not apply any more, right?



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

    I think we should move this check to `Master::QuotaHandler::set()` and even 
convert to `CHECK_` because it's an internal invarian at this moment. It would 
be nice to reuse this function for other quota requests, means we do not need 
any checks here.



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

    I think we should agree on terminology: "quota request", "Quota Request", 
or "QuotaRequest".
    
    We should be consistent here and everywhere (also in other RRs).



src/master/quota_handler.cpp (lines 67 - 68)
<https://reviews.apache.org/r/39285/#comment161606>

    This fits in one line.



src/master/quota_handler.cpp (lines 71 - 72)
<https://reviews.apache.org/r/39285/#comment161609>

    How about initializing `role` with QuotaInfo.role? Or we agreed not to send 
role as part of the JSON request?



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

    Let's be consistent and use present simple everywhere : ).
    
    s/Checking that Resource does not contain non-relevant fields./Check that 
Resources does not contain fields non-relevant for quota.



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

    I think we can kill this comment.



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

    And this one.



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

    And this one.



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

    The code does something different to what the comment says : ).



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

    s/all resources are scalar/the resource is scalar



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

    This sounds incomplete to me. Maybe "includes" is better?



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

    We are not consistent in BadRequet error messages, but how about 
"Attempting to set quota for an unknown role '<role>'"?



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

    s/Quota Info/QuotaInfo



src/master/quota_handler.cpp (lines 156 - 160)
<https://reviews.apache.org/r/39285/#comment161614>

    This is a bug: not in sync with the way we store quotas:
    ```
    master->quotas[quota.role()] = quota;
    ```.



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

    We do not support update currently, so the message is misleading. How about 
"Quota cannot be set for a role that already has quota"?



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

    Why this newline?


- Alexander Rukletsov


On Oct. 22, 2015, 4:38 a.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2015, 4:38 a.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
> 
>

Reply via email to