----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48268/#review136955 -----------------------------------------------------------
Mostly comments around: - Similar to comments from @haosdent abstract the common parts in a method `_setQuota`. - Remove some additional tests. src/internal/devolve.hpp (line 48) <https://reviews.apache.org/r/48268/#comment202065> Can you move this after L59 with a newline before and after? It would be good to group all similar `*Request` messages together thereafter. Also, we won't be needing this overload once MESOS-5593 is resolved since we would then directly devolve to `master::Call`. Can you also add a TODO to remove this once MESOS-5593 is resolved. src/internal/evolve.hpp (lines 59 - 61) <https://reviews.apache.org/r/48268/#comment202075> We won't be needing them. See my comments on the tests. src/master/quota_handler.cpp (line 278) <https://reviews.apache.org/r/48268/#comment202074> Can we abstract out the code after this in a function `_setQuota` that both can use? ```cpp Future<http::Response> Master::QuotaHandler::_setQuota( const QuotaRequest& request, const Option<string>& principal) const { ... } ``` Makes sense? src/tests/api_tests.cpp (lines 217 - 218) <https://reviews.apache.org/r/48268/#comment202076> Why do we need this comment here? src/tests/api_tests.cpp (line 223) <https://reviews.apache.org/r/48268/#comment202077> Can you move this down to L241 where it is used? src/tests/api_tests.cpp (lines 225 - 226) <https://reviews.apache.org/r/48268/#comment202082> Move this comment before L235 where you use `FORCE` and then just do `set_force(true)` src/tests/api_tests.cpp (line 227) <https://reviews.apache.org/r/48268/#comment202081> Kill this variable src/tests/api_tests.cpp (line 229) <https://reviews.apache.org/r/48268/#comment202079> Kill this comment src/tests/api_tests.cpp (lines 254 - 410) <https://reviews.apache.org/r/48268/#comment202073> Kill these. The existing Quota handler workflow already tests this. We don't need to exercise those tests again. - Anand Mazumdar On June 6, 2016, 9:55 a.m., Abhishek Dasgupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48268/ > ----------------------------------------------------------- > > (Updated June 6, 2016, 9:55 a.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. This call > does not return v1::Response message, instead it returns > http::Response. > > > Diffs > ----- > > include/mesos/v1/quota/quota.hpp PRE-CREATION > src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc > src/internal/devolve.hpp 00842bb0de1dd587f2b47c79f17c0e7bd7f51189 > src/internal/devolve.cpp 4233246c6838f85189f1b4c7e66d2bc0a3bf5408 > src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 > src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 > src/master/http.cpp 824c6e5adcebc83d1ec742c9bd036a8f24c9a343 > src/master/master.hpp 846edf37d13b44093832ca3d184426b403174b35 > src/master/quota_handler.cpp 06cf0d3f24844ec9e884237db3f33145ff406e80 > src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 > > 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 > >
