----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36036/#review89870 -----------------------------------------------------------
Looks good to me, but I'll let Vinod give the final approval. I was wondering if there could be any upgrade issues, but I don't think so, since it's an optional field and we aren't even reading reading it anywhere, just setting it for new masters and sharing it with frameworks, ZK, and the registrar, all of which either ignore it or just store it. src/tests/master_contender_detector_tests.cpp (line 183) <https://reviews.apache.org/r/36036/#comment142726> I'm a little wary of using `auto` here, because it's not immediately obvious to me what type `masterInfo` is from this line. I would have thought it was a `MasterInfo`, until I looked 20 lines above and realized it was actually `Option<MasterInfo>`. Our style guide says "The main goal is to increase code readability. This is safely the case if the exact same type omitted on the left is already fully stated on the right." Interpret that how you will. src/tests/master_contender_detector_tests.cpp (lines 184 - 186) <https://reviews.apache.org/r/36036/#comment142727> From the google primer, "Usually `EXPECT_*` are preferred, as they allow more than one failures to be reported in a test. However, you should use `ASSERT_*` if it doesn't make sense to continue when the assertion in question fails." If somebody were to add more checks after yours (e.g. for new MasterInfo fields), it would make sense for the test to continue with other checks that don't care whether MasterInfo has a version. - Adam B On June 29, 2015, 11:28 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36036/ > ----------------------------------------------------------- > > (Updated June 29, 2015, 11:28 p.m.) > > > Review request for mesos, Anand Mazumdar, Isabel Jimenez, and Vinod Kone. > > > Bugs: MESOS-2957 > https://issues.apache.org/jira/browse/MESOS-2957 > > > Repository: mesos > > > Description > ------- > > Jira: MESOS-2957 > > Adds an optional version string to the `MasterInfo` protobuf, > to simplify handling versioning in HTTP API calls. > > The version string is taken from <mesos/version.hpp> which is the > same used when starting up Master (master/main.cpp). > > I've added a simple test on the back of the `MasterDetector` test, > as this was the place where we would expect the `MasterInfo` to > have been fully populated by real production code (as opposed to > other places, where it is actually handled by mocks). > > > Diffs > ----- > > include/mesos/mesos.proto 0ebe5d3a5ae033419482c28be80b09d277e0ff02 > src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c > src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d > src/tests/master_contender_detector_tests.cpp > d7a3b46b2e437818631064ae34317e49c9aa3748 > > Diff: https://reviews.apache.org/r/36036/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
