----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48094/#review138342 -----------------------------------------------------------
Just a few more minor comments. Looks pretty good otherwise! include/mesos/master/master.proto (lines 286 - 287) <https://reviews.apache.org/r/48094/#comment203503> The `GetRoles` message has much more information than this. Let's be a bit more explicit. Also, the previous message had some typos. How about: ```cpp // Provides information about every role that is on the role whitelist (if enabled), has one or more registered frameworks or has a non-default weight or quota. ``` include/mesos/mesos.proto (line 2015) <https://reviews.apache.org/r/48094/#comment203515> hmm, thinking about it more, we can just do: `repeated FrameworkID frameworks` now that we have the well define protobuf object instead of returning a `string`. include/mesos/v1/master/master.proto (lines 286 - 287) <https://reviews.apache.org/r/48094/#comment203504> Ditto as the unversioned proto comment. src/master/http.cpp (line 1417) <https://reviews.apache.org/r/48094/#comment203510> hmm, can we include some information as to why we ended up duplicating most of the logic of `_roles` for posterity? How about: ```cpp This duplicates the functionality offered by `roles()`. This was necessary as the JSON object returned by `roles()` was not specified in a formal way i.e. via a corresponding protobuf object and would have been very hard to convert back into a `Resource` object. ``` src/master/http.cpp (line 1430) <https://reviews.apache.org/r/48094/#comment203511> s/getRole/role Also move this down inside the `foreach` loop on L1449 src/master/http.cpp (lines 1469 - 1472) <https://reviews.apache.org/r/48094/#comment203518> Can't you just do: ```cpp getRole.mutable_resources()->CopyFrom(role.get()->resources); ``` src/tests/api_tests.cpp (line 317) <https://reviews.apache.org/r/48094/#comment203521> Can you also include an assert for the expected number of roles in the response i.e. 3? ```cpp ASSERT_EQ(3, v1Response->get_roles().roles().size()); ``` src/tests/api_tests.cpp (lines 319 - 322) <https://reviews.apache.org/r/48094/#comment203527> Can you directly compare the `Resource` object itself instead of comparing some resource atrributes? - Anand Mazumdar On June 17, 2016, 8:30 a.m., Abhishek Dasgupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48094/ > ----------------------------------------------------------- > > (Updated June 17, 2016, 8:30 a.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-5494 > https://issues.apache.org/jira/browse/MESOS-5494 > > > Repository: mesos > > > Description > ------- > > Implemented GET_ROLES Call in v1 master API. > > > Diffs > ----- > > include/mesos/master/master.proto 3b85b8130f1a292f3574214fcf7761c891f57f22 > include/mesos/mesos.proto d3cfdd0a583bbd0f5ec11c87ea29e649a97450c5 > include/mesos/v1/master/master.proto > 5a6bbeb0f9b57d03803e366df32e04e9598e00ed > include/mesos/v1/mesos.proto dca0c2c8a468345cb18b4ced889fc3db0324ca1d > src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d > src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 > src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 > > Diff: https://reviews.apache.org/r/48094/diff/ > > > Testing > ------- > > On Ubuntu 16.04: > > sudo GTEST_FILTER="*MasterAPITest.*" make -j2 check > > > Thanks, > > Abhishek Dasgupta > >
