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