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



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

    Why is this protected?



src/master/quota.hpp (lines 35 - 38)
<https://reviews.apache.org/r/39285/#comment164715>

    let's indent the list.



src/master/quota.cpp (lines 36 - 37)
<https://reviews.apache.org/r/39285/#comment164717>

    Why are these inside the namespaces as opposed to above?



src/master/quota.cpp (line 43)
<https://reviews.apache.org/r/39285/#comment164737>

    shouldn't this check that there is at least 1 resource?



src/master/quota.cpp (line 49)
<https://reviews.apache.org/r/39285/#comment164730>

    It's not necessarily a "request" at this point right? It's an arbitrary 
`QuotaInfo`?
    Same below



src/master/quota.cpp (line 70)
<https://reviews.apache.org/r/39285/#comment164731>

    s/Check all/Check that all/



src/master/quota.cpp (lines 72 - 83)
<https://reviews.apache.org/r/39285/#comment164733>

    your previous error messages specified what the requirement was. These 
specify what is wrong. Please stay consistent.



src/master/quota.cpp (line 78)
<https://reviews.apache.org/r/39285/#comment164732>

    new line



src/master/quota.cpp (line 81)
<https://reviews.apache.org/r/39285/#comment164734>

    trailing whitespace



src/master/quota.cpp (line 84)
<https://reviews.apache.org/r/39285/#comment164735>

    What do you think about a space after the comma?



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

    Should we call this something like `createQuotaInfo` to math other similar 
factory functions?



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

    specifically: it's being constructed from json here.
    Might be valuable to log the request, if we bother to log the action?



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

    How come we don't leverage the validation routine you introduced in this 
patch here?



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

    Expected, followed by actual



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

    should we provide the query string that we couldn't decode?
    Same for error messages below.



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

    Why the hashmap `Option<T> get()` pattern versus `contains(key)`?



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

    same here regarding the get pattern question



src/master/quota_handler.cpp (lines 122 - 126)
<https://reviews.apache.org/r/39285/#comment164744>

    If we call it `createQuotaInfo` then we can stay consistent in the 
terminology (for example here we're already using extract and convert to talk 
about the same action)



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

    Here you reference the kind of request (i.e. `set quota`). In the other 
messages you don't. Let's stay consistent. I actually like referencing the 
request type like you did in this error message :-)



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

    s/Check we/Check that we/


- Joris Van Remoortere


On Nov. 10, 2015, 6:20 p.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2015, 6:20 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 ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> -------
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Reply via email to