----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39285/#review103539 -----------------------------------------------------------
src/master/master.hpp (line 878) <https://reviews.apache.org/r/39285/#comment161593> s/Check/Checks s/Request/request Do we capitalize "quota"? src/master/master.hpp (lines 879 - 882) <https://reviews.apache.org/r/39285/#comment161595> 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) <https://reviews.apache.org/r/39285/#comment161594> 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) <https://reviews.apache.org/r/39285/#comment161604> This does not apply any more, right? src/master/quota_handler.cpp (line 60) <https://reviews.apache.org/r/39285/#comment161616> 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) <https://reviews.apache.org/r/39285/#comment161605> 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) <https://reviews.apache.org/r/39285/#comment161606> This fits in one line. src/master/quota_handler.cpp (lines 71 - 72) <https://reviews.apache.org/r/39285/#comment161609> 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) <https://reviews.apache.org/r/39285/#comment161621> 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) <https://reviews.apache.org/r/39285/#comment161618> I think we can kill this comment. src/master/quota_handler.cpp (line 93) <https://reviews.apache.org/r/39285/#comment161619> And this one. src/master/quota_handler.cpp (line 97) <https://reviews.apache.org/r/39285/#comment161620> And this one. src/master/quota_handler.cpp (line 102) <https://reviews.apache.org/r/39285/#comment161610> The code does something different to what the comment says : ). src/master/quota_handler.cpp (line 115) <https://reviews.apache.org/r/39285/#comment161617> s/all resources are scalar/the resource is scalar src/master/quota_handler.cpp (line 118) <https://reviews.apache.org/r/39285/#comment161623> This sounds incomplete to me. Maybe "includes" is better? src/master/quota_handler.cpp (line 127) <https://reviews.apache.org/r/39285/#comment161624> 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) <https://reviews.apache.org/r/39285/#comment161612> s/Quota Info/QuotaInfo src/master/quota_handler.cpp (lines 156 - 160) <https://reviews.apache.org/r/39285/#comment161614> 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) <https://reviews.apache.org/r/39285/#comment161615> 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) <https://reviews.apache.org/r/39285/#comment161613> 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 > >
