> 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 > >
