----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48268/#review138519 -----------------------------------------------------------
include/mesos/v1/master/master.proto (lines 164 - 165) <https://reviews.apache.org/r/48268/#comment203722> s/SET_QUOTA call to set/Sets include/mesos/v1/master/master.proto (lines 167 - 168) <https://reviews.apache.org/r/48268/#comment203721> Kill this, this is redundant. The reader can see the documentation of `QuotaRequest` message from it's corresponding protobuf. src/Makefile.am (line 718) <https://reviews.apache.org/r/48268/#comment203704> Can we do this change in a separate patch? This modification is outside the scope of implementing the `SET_QUOTA` call. For now, just include the quota header pb in this patch and post another patch thereafter. src/master/master.hpp (line 959) <https://reviews.apache.org/r/48268/#comment203709> hmm.. there is already an existing overload called `set()`. Can we just use `set()` as the function name? Also, since this function is already inside `QuotaHandler`, `setQuota` is redundant. src/master/master.hpp (line 962) <https://reviews.apache.org/r/48268/#comment203711> What do we need the argument `ContentType` for if we just return a `OK()`? I see that you don't use this parameter in the definition at all? I wonder how this escaped the `-Wunused-parameter warning`. src/master/master.hpp (line 1038) <https://reviews.apache.org/r/48268/#comment203714> s/_setQuota/_set src/master/master.hpp (line 1039) <https://reviews.apache.org/r/48268/#comment203705> const reference please. src/master/quota_handler.cpp (lines 272 - 274) <https://reviews.apache.org/r/48268/#comment203707> 4 spaces indent please src/master/quota_handler.cpp (line 277) <https://reviews.apache.org/r/48268/#comment203713> Nit: Kill this new line and group these checks together? src/master/quota_handler.cpp (lines 280 - 282) <https://reviews.apache.org/r/48268/#comment203710> Can we just inline the previous statement? ```cpp return _setQuota(call.set_quota().quota_request, principal) ``` Also, it's a good practice to capture aliases by const reference to avoid the overhead of copying. (`const QuotaRequest&`) src/master/quota_handler.cpp (line 290) <https://reviews.apache.org/r/48268/#comment203715> Not yours: I am fine with removing this self explanatory comment. src/master/quota_handler.cpp (line 304) <https://reviews.apache.org/r/48268/#comment203712> no space between request and ':' src/tests/api_tests.cpp (line 17) <https://reviews.apache.org/r/48268/#comment203716> Kill this, you already included this 2 lines later? src/tests/api_tests.cpp (line 48) <https://reviews.apache.org/r/48268/#comment203717> Kill this src/tests/api_tests.cpp (line 64) <https://reviews.apache.org/r/48268/#comment203723> Do you need it? src/tests/api_tests.cpp (line 753) <https://reviews.apache.org/r/48268/#comment203719> Why is this scoped in its own block? Can we just build it in the main block now that we don't have more tests? src/tests/api_tests.cpp (line 758) <https://reviews.apache.org/r/48268/#comment203720> Nit: Newline before. src/tests/api_tests.cpp (lines 762 - 763) <https://reviews.apache.org/r/48268/#comment203718> Why not just? ```cpp quotaRequest.mutable_guarantee()->CopyFrom(quotaResources); ``` - Anand Mazumdar On June 19, 2016, 5:46 p.m., Abhishek Dasgupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48268/ > ----------------------------------------------------------- > > (Updated June 19, 2016, 5:46 p.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-5509 > https://issues.apache.org/jira/browse/MESOS-5509 > > > Repository: mesos > > > Description > ------- > > Implemented SET_QUOTA Call in v1 master API. > > > Diffs > ----- > > include/mesos/v1/master/master.proto > abcf628eea7922cdb323ec135ec48e9b11209608 > src/Makefile.am a4931560f1a5b3fbe41ea181477341d3ac459b58 > src/master/http.cpp 148324e2c9d52f8211df4cead783e5a2979a45fe > src/master/master.hpp 50230397dcf52219fd4ed98dd1107bf53790478d > src/master/quota_handler.cpp 567f1c2cc59b859227a8b48c6086ce3f8049f14d > src/tests/api_tests.cpp 5d10533858cf1c512e00dbe6d5fab1f6401687bf > > Diff: https://reviews.apache.org/r/48268/diff/ > > > Testing > ------- > > On Ubuntu 16.04: > sudo GTEST_FILTER="*MasterAPITest.SetQuota*" make -j4 check > > [==========] Running 2 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 2 tests from ContentType/MasterAPITest > [ RUN ] ContentType/MasterAPITest.SetQuota/0 > [ OK ] ContentType/MasterAPITest.SetQuota/0 (129 ms) > [ RUN ] ContentType/MasterAPITest.SetQuota/1 > [ OK ] ContentType/MasterAPITest.SetQuota/1 (98 ms) > [----------] 2 tests from ContentType/MasterAPITest (227 ms total) > > [----------] Global test environment tear-down > [==========] 2 tests from 1 test case ran. (236 ms total) > [ PASSED ] 2 tests. > > > Thanks, > > Abhishek Dasgupta > >
