----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39285/#review106122 -----------------------------------------------------------
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). src/master/quota.cpp (line 78) <https://reviews.apache.org/r/39285/#comment164814> Any reason why you switch to "must" from "may" here? src/master/quota.cpp (line 81) <https://reviews.apache.org/r/39285/#comment164817> Do we prohibit empty roles? src/master/quota_handler.cpp (line 146) <https://reviews.apache.org/r/39285/#comment164816> You call it "request query string" above, any reason you change the name? src/master/quota_handler.cpp (line 153) <https://reviews.apache.org/r/39285/#comment164815> 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"? - Alexander Rukletsov 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 > >
