> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote: > > LGTM.. Just some nits around: > > > > - using `const` for test strings. > > - reducing jaggedness for some blocks. > > > > Also, a query regarding just accepting `true/false` as `force` field values.
Good points, thanks for the review, Anand! > On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote: > > src/master/quota_handler.cpp, line 273 > > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line273> > > > > nit: use backticks (`force`) > > s/flag/field I'm not sure we backtick flags, do we? AFAIK, we do that for object, variables, function names, types. > On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote: > > src/master/quota_handler.cpp, line 299 > > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line299> > > > > hmm, why don't we do a `using google::protobuf::RepeatedPtrField` and > > get rid of all this jaggedness ? Yeah, and also `#include` it properly : ) > On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote: > > src/master/quota_handler.cpp, line 338 > > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line338> > > > > I like the less jagged version more here. It's also more consistent > > with other similar strings in the function. What do you think ? It's a known fact, that jaggedness is very subjective. I would argue, that wrapping error message after `BadRequest` is less jagged and more readable, because more there is more space for the message itself. Consistency is important, hence the following review: https://reviews.apache.org/r/41515/ > On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote: > > src/master/quota_handler.cpp, line 353 > > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line353> > > > > hmmm .. Should we return a `BadRequest` for all other non-allowed > > values of `force` other then `true` or `false` ? I was thinking about the [Postel's law](https://en.wikipedia.org/wiki/Robustness_principle), but maybe you are right and we should not interpret "force:t", "force:1", "force:god-damn-yes" as `false`, I'll fix that. > On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote: > > src/tests/master_quota_tests.cpp, line 199 > > <https://reviews.apache.org/r/41514/diff/1/?file=1168676#file1168676line199> > > > > Not related to this review : Why don't we make all these `badRequest` > > expected strings `const` ? Yeah, let's fix it. Thanks! - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41514/#review111086 ----------------------------------------------------------- On Dec. 17, 2015, 2:52 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41514/ > ----------------------------------------------------------- > > (Updated Dec. 17, 2015, 2:52 p.m.) > > > Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and > Joris Van Remoortere. > > > Bugs: MESOS-3960 > https://issues.apache.org/jira/browse/MESOS-3960 > > > Repository: mesos > > > Description > ------- > > POST request to "/quota" requires a single JSON object as opposed to > key-value pairs encoded in a string. > > > Diffs > ----- > > src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 > src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 > > Diff: https://reviews.apache.org/r/41514/diff/ > > > Testing > ------- > > make check on Mac OS 10.10.4 > > > Thanks, > > Alexander Rukletsov > >
