mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joerg Schad" <jo...@mesosphere.io>
Subject Re: Review Request 39285: Added Quota Request Validation.
Date Wed, 11 Nov 2015 19:41:15 GMT


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > One thing I think is not entirely clean is repetition of some validation checks.
For example, you check whether a role is set twice: while constructing a `QuotaInfo` instance
and while validating it. I think we can sacrifice one check, for example in the `createQuotaInfo()`
function, and leave a comment there that an object should be validated afterwards. Alternatively,
we can do validation inside `createQuotaInfo()`, but this seems inconsistent to the way other
endpoint handlers operate (e.g. maintenance, dynamic reservations).

Unfortunately, we need both checks.
a) as validate quotaInfo is a general check for quotaInfo we should validate it there
b) in createQuotaInfo I need to extract the role, therefore I need to verify that there is
at least one resource


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota.cpp, line 81
> > <https://reviews.apache.org/r/39285/diff/19/?file=1123437#file1123437line81>
> >
> >     Any reason why you switch to "must" from "may" here?

Because before it was about stuff which is forbidden, this is about stuff which is required....


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota.cpp, line 84
> > <https://reviews.apache.org/r/39285/diff/19/?file=1123437#file1123437line84>
> >
> >     Do we prohibit empty roles?

After discussion with Joris yes.


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 148
> > <https://reviews.apache.org/r/39285/diff/19/?file=1123438#file1123438line148>
> >
> >     You call it "request query string" above, any reason you change the name?

I considered both, the reason for this choice:
Above it is about stuff inside the query string, the last two checks are more concerned with
the request itself (being for role). The alternative would have been something like "Quota
request query string (...) including resource with an unknown role ".." which I felt not really
parsable...


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 155
> > <https://reviews.apache.org/r/39285/diff/19/?file=1123438#file1123438line155>
> >
> >     Do you want to phrase it similar to error messages above? Something like: "Failed
to fulfill quota set request ('...') for a role with already set quota"?

I used Failed if some other operation failed, this is more like "Missing 'resources' in set
quota request query string".
If you want I can change this to "Unable to fullfill quota set request...."


- Joerg


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


On Nov. 11, 2015, 5:35 p.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2015, 5:35 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
> 
>


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