----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30996/#review72651 -----------------------------------------------------------
Looking good, but I don't know why this would go into messages.proto instead of mesos.proto. src/common/parse.hpp <https://reviews.apache.org/r/30996/#comment118697> Please add another blank line here to space out the top-level functions. src/messages/messages.hpp <https://reviews.apache.org/r/30996/#comment118701> If DisabledEndpoints/Endpoint go into mesos.proto, then you can also move this to type_utils.hpp src/messages/messages.hpp <https://reviews.apache.org/r/30996/#comment118695> Please add another blank line here, since we're at the top-level scope. src/messages/messages.proto <https://reviews.apache.org/r/30996/#comment118698> Perhaps this should go into mesos.proto, along with ACL/ACLs, Credential/Credentials, and other datatype protobufs. Typically messages.proto is used for internal messages (which end in Message). src/messages/messages.proto <https://reviews.apache.org/r/30996/#comment118699> Perhaps this should be `repeated Endpoint endpoints = 1;` Then you could have a separate `message Endpoint { required string value = 1; }` Maybe that's overkill, but I could imagine us wanting Endpoint for other things in the future, and separating them out like this would make it easier to add more per-endpoint fields. What do you think? - Adam B On Feb. 13, 2015, 8:02 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30996/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 8:02 a.m.) > > > Review request for mesos, Adam B, Till Toenshoff, and Vinod Kone. > > > Bugs: MESOS-2333 > https://issues.apache.org/jira/browse/MESOS-2333 > > > Repository: mesos > > > Description > ------- > > Adds a `DisabledEndpoints` protobuf message, which will allow the user give a > list of endpoints which should be disabled. > > > Diffs > ----- > > src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc > src/messages/messages.hpp 25769b797ec9bd1a1c348c467616aef8407c45d6 > src/messages/messages.proto 58484ae45071a80afd2b11803dd66a88f88ad9ed > > Diff: https://reviews.apache.org/r/30996/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
