This is an automatically generated e-mail. To reply, visit:

src/master/master.hpp (line 878)

    Do we capitalize "quota"?

src/master/master.hpp (lines 879 - 882)

    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)

    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)

    This does not apply any more, right?

src/master/quota_handler.cpp (line 60)

    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)

    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)

    This fits in one line.

src/master/quota_handler.cpp (lines 71 - 72)

    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)

    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)

    I think we can kill this comment.

src/master/quota_handler.cpp (line 93)

    And this one.

src/master/quota_handler.cpp (line 97)

    And this one.

src/master/quota_handler.cpp (line 102)

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

src/master/quota_handler.cpp (line 115)

    s/all resources are scalar/the resource is scalar

src/master/quota_handler.cpp (line 118)

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

src/master/quota_handler.cpp (line 127)

    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)

    s/Quota Info/QuotaInfo

src/master/quota_handler.cpp (lines 156 - 160)

    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)

    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)

    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