mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 36036: Add `version` string to MasterInfo
Date Tue, 30 Jun 2015 07:26:55 GMT

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


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