> On June 30, 2015, 7:26 a.m., Adam B wrote: > > 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.
Correct - it's currently unused... mostly because, until a couple of hours ago, it didn't exist ;) > On June 30, 2015, 7:26 a.m., Adam B wrote: > > src/tests/master_contender_detector_tests.cpp, line 183 > > <https://reviews.apache.org/r/36036/diff/1/?file=995701#file995701line183> > > > > 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. I beg to disagree and dance with Scott Mayers on this one :) (Item 5: Prefer auto to explicit declarations) Also, remember this is a test: I think just on the line below if memory serves I do a ASSERT_SOME() which is a bit of a giveaway :) > On June 30, 2015, 7:26 a.m., Adam B wrote: > > src/tests/master_contender_detector_tests.cpp, lines 184-186 > > <https://reviews.apache.org/r/36036/diff/1/?file=995701#file995701line184> > > > > 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. I would generally agree, but on my most recent review (where I had used an EXPECT_*), Vinod noted that, as it was the last check, an ASSERT_* was preferable. No personal preference - I'm happy to go with the flow. - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36036/#review89870 ----------------------------------------------------------- On June 30, 2015, 6:28 a.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36036/ > ----------------------------------------------------------- > > (Updated June 30, 2015, 6:28 a.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 > >
