mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 36036: Add `version` string to MasterInfo
Date Tue, 30 Jun 2015 07:41:52 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message