----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55710/#review165217 -----------------------------------------------------------
Looks good, the main issue is that it seems error-prone to store both the booleans and the raw capabilities within the struct in a non-const manner, since they can go out of sync. Left some suggestions below. include/mesos/master/master.proto (line 310) <https://reviews.apache.org/r/55710/#comment237042> I don't think you need the `agent_` prefix, given we're within `Agent` here, any reason you added it? src/common/protobuf_utils.hpp (lines 164 - 165) <https://reviews.apache.org/r/55710/#comment237047> This is unfortunate since the bool and the repeated field need to be kept consistent but there's nothing enforcing this. How about a function that generates the repeated field? ``` google::protobuf::RepeatedPtrField<SlaveInfo::Capability> toRepeatedField() const { ...; } ``` Alternatively, store the capabilities as you did here, but make the booleans functions that loop over the capabilities: ``` bool multiRole() const { ...; } ``` src/common/protobuf_utils.cpp (lines 654 - 658) <https://reviews.apache.org/r/55710/#comment237048> Can't you just do a single CopyFrom here? ``` agent.mutable_agent_capabilities()->CopyFrom( slave.capabilities.capabilities); ``` src/master/http.cpp (lines 136 - 137) <https://reviews.apache.org/r/55710/#comment237045> Can you put each argument on its own line? ``` static void json( JSON::StringWriter* writer, const SlaveInfo::Capability& capability) ``` src/tests/master_tests.cpp (line 4087) <https://reviews.apache.org/r/55710/#comment237050> We can just use the `->` operator instead of `.get().`, note that a lot of the existing tests still need to be swept to clean them up. - Benjamin Mahler On Feb. 9, 2017, 7:22 a.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55710/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2017, 7:22 a.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and > Michael Park. > > > Bugs: MESOS-6902 > https://issues.apache.org/jira/browse/MESOS-6902 > > > Repository: mesos > > > Description > ------- > > Master should be able to reflect agent capabilities via `/state`(v0) > and `getState`(v1) endpoints. > > > Diffs > ----- > > include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017 > include/mesos/v1/master/master.proto > cfdca7400d98233c6320eebd8784e4f51c43ebbd > src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5 > src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a > src/master/http.cpp a598488296d4616c0126aa3bd4d1d7e7a6e439fe > src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18 > > Diff: https://reviews.apache.org/r/55710/diff/ > > > Testing > ------- > > make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities" > [ OK ] MasterTest.StateEndpointAgentCapabilities (85 ms) > [----------] 1 test from MasterTest (94 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (122 ms total) > > make check on Ubuntu 14.04 > > > Thanks, > > Jay Guo > >
