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