You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/06/30 08:28:07 UTC
Review Request 36036: Add `version` string to MasterInfo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/
-----------------------------------------------------------
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
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review90021
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36036]
All tests passed.
- Mesos ReviewBot
On June 30, 2015, 11:23 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36036/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 11:23 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 5ab3c4a305562af16af804e7ab77e576b96e468d
> src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
> src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
> src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/36036/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 1, 2015, 1:54 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 369
> > <https://reviews.apache.org/r/36036/diff/2/?file=996271#file996271line369>
> >
> > s/Release/Version/
as per @adam comment, I'm removing this from the log
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review89991
-----------------------------------------------------------
On June 30, 2015, 11:23 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36036/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 11:23 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 5ab3c4a305562af16af804e7ab77e576b96e468d
> src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
> src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
> src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/36036/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review89991
-----------------------------------------------------------
Ship it!
src/common/protobuf_utils.cpp (line 150)
<https://reviews.apache.org/r/36036/#comment142912>
s/5051/5050/
src/master/master.cpp (line 369)
<https://reviews.apache.org/r/36036/#comment142914>
s/Release/Version/
- Vinod Kone
On June 30, 2015, 11:23 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36036/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 11:23 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 5ab3c4a305562af16af804e7ab77e576b96e468d
> src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
> src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
> src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/36036/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 1, 2015, 7:48 a.m., Adam B wrote:
> > src/common/protobuf_utils.cpp, lines 147-149
> > <https://reviews.apache.org/r/36036/diff/2/?file=996269#file996269line147>
> >
> > Could join these paragraphs. I think they're related enough, and the first one is short.
Unless you feel strongly, I'd rather like to leave them as separate paragraphs: I find big blocks of text less readable in apidocs.
> On July 1, 2015, 7:48 a.m., Adam B wrote:
> > src/common/protobuf_utils.cpp, lines 155-156
> > <https://reviews.apache.org/r/36036/diff/2/?file=996269#file996269line155>
> >
> > Please capitalize the sentences/statements in your param/returns.
> > s/the process/The process/
> > s/a fully-formed/A fully formed/ (also, no hyphen necessary)
just FYI the javadoc style guide actually does *not* require capitalization
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide
Not sure whether we dictate otherwise in our guide.
I've capitalized here, as it was really not worth to make a fuss about :)
> On July 1, 2015, 7:48 a.m., Adam B wrote:
> > src/master/master.cpp, lines 368-369
> > <https://reviews.apache.org/r/36036/diff/2/?file=996271#file996271line368>
> >
> > No need to log the version number here. `src/master/main.cpp` already logs the version, build, and git tag/sha closer to the top of the master log, in a more greppable format:
> > main.cpp:181] Build: 2015-05-05 06:16:35 by root
> > main.cpp:183] Version: 0.22.1
> > main.cpp:186] Git tag: 0.22.1
> > main.cpp:190] Git SHA: d6309f92a7f9af3ab61a878403e3d9c284ea87e0
> >
> > And if you do include it, I don't think it needs to be inside hostname's parentheses.
removed
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review90024
-----------------------------------------------------------
On June 30, 2015, 11:23 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36036/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 11:23 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 5ab3c4a305562af16af804e7ab77e576b96e468d
> src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
> src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
> src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/36036/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review90024
-----------------------------------------------------------
Ship it!
Looks good. Remove the unnecessary logging, and clean up the comment, then I'd call it shippable.
src/common/protobuf_utils.cpp (lines 147 - 149)
<https://reviews.apache.org/r/36036/#comment142955>
Could join these paragraphs. I think they're related enough, and the first one is short.
src/common/protobuf_utils.cpp (lines 155 - 156)
<https://reviews.apache.org/r/36036/#comment142953>
Please capitalize the sentences/statements in your param/returns.
s/the process/The process/
s/a fully-formed/A fully formed/ (also, no hyphen necessary)
src/master/master.cpp (lines 368 - 369)
<https://reviews.apache.org/r/36036/#comment142950>
No need to log the version number here. `src/master/main.cpp` already logs the version, build, and git tag/sha closer to the top of the master log, in a more greppable format:
main.cpp:181] Build: 2015-05-05 06:16:35 by root
main.cpp:183] Version: 0.22.1
main.cpp:186] Git tag: 0.22.1
main.cpp:190] Git SHA: d6309f92a7f9af3ab61a878403e3d9c284ea87e0
And if you do include it, I don't think it needs to be inside hostname's parentheses.
- Adam B
On June 30, 2015, 4:23 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36036/
> -----------------------------------------------------------
>
> (Updated June 30, 2015, 4:23 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 5ab3c4a305562af16af804e7ab77e576b96e468d
> src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
> src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
> src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/36036/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/
-----------------------------------------------------------
(Updated July 1, 2015, 7:56 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 (updated)
-----
include/mesos/mesos.proto 5ab3c4a305562af16af804e7ab77e576b96e468d
src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
Diff: https://reviews.apache.org/r/36036/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/
-----------------------------------------------------------
(Updated June 30, 2015, 11:23 p.m.)
Review request for mesos, Anand Mazumdar, Isabel Jimenez, and Vinod Kone.
Changes
-------
Wild goose chase to discover a bug - initialized the version in Master::Master() c'tor (NOT initialize()) to avoid failures.
Implemented all other comments from Vinod.
All tests pass.
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 (updated)
-----
include/mesos/mesos.proto 5ab3c4a305562af16af804e7ab77e576b96e468d
src/common/protobuf_utils.cpp f0642ba12b914889a876d1d1c49a2c242006433c
src/common/type_utils.cpp f7b21c6abc392fec0514224ff02a00e772389f5d
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
Diff: https://reviews.apache.org/r/36036/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On June 30, 2015, 6:01 p.m., Vinod Kone wrote:
> > src/common/protobuf_utils.cpp, line 145
> > <https://reviews.apache.org/r/36036/diff/1/?file=995699#file995699line145>
> >
> > We should've added a comment on this method that this is only used by the StandaloneMasterDetector (used in tests and outside tests when ZK is not used). For example when we start a slave with --master=master@127.0.0.1:5051, since the slave (and consequently its detector) doesn't have enough information about MasterInfo, it tries to construct it based on the available information (UPID). Mind adding this comment?
> >
> > So, I don't think it makes sense for us to add the version here, because the slave/framework (detector) doesn't know what version the master is running. What you are doing is setting the version of slave (or libmesos in case of framework) in MasterInfo!
LoL - talk about the need for more documentation :)
Sure, will do
(but are you totally sure about this? I'm almost sure I did the work here when I did test the JSON/PB thing and it did work in ZK too? but can't really remember if I did modify this method too.)
> On June 30, 2015, 6:01 p.m., Vinod Kone wrote:
> > src/tests/master_contender_detector_tests.cpp, lines 185-186
> > <https://reviews.apache.org/r/36036/diff/1/?file=995701#file995701line185>
> >
> > Per my comment above, a test using StandaloneMasterDetector shouldn't be testing version.
> >
> > So kill this block.
> >
> > I would recommend adding this check in a test that uses ZooKeeperMasterDetector as thats what production clusters use. See MasterZookeerTest in master_tests.cpp for inspiration.
done (removing test on version) but is it ok if I leave there the assertion on a non-None MasterInfo, though?
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review89933
-----------------------------------------------------------
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
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review89933
-----------------------------------------------------------
src/common/protobuf_utils.cpp (line 145)
<https://reviews.apache.org/r/36036/#comment142825>
We should've added a comment on this method that this is only used by the StandaloneMasterDetector (used in tests and outside tests when ZK is not used). For example when we start a slave with --master=master@127.0.0.1:5051, since the slave (and consequently its detector) doesn't have enough information about MasterInfo, it tries to construct it based on the available information (UPID). Mind adding this comment?
So, I don't think it makes sense for us to add the version here, because the slave/framework (detector) doesn't know what version the master is running. What you are doing is setting the version of slave (or libmesos in case of framework) in MasterInfo!
src/common/protobuf_utils.cpp (line 162)
<https://reviews.apache.org/r/36036/#comment142826>
Kill this. See above.
Do this in Master::initialize() where the MasterInfo is created by the master.
src/tests/master_contender_detector_tests.cpp (lines 185 - 186)
<https://reviews.apache.org/r/36036/#comment142828>
Per my comment above, a test using StandaloneMasterDetector shouldn't be testing version.
So kill this block.
I would recommend adding this check in a test that uses ZooKeeperMasterDetector as thats what production clusters use. See MasterZookeerTest in master_tests.cpp for inspiration.
- Vinod Kone
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
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review89876
-----------------------------------------------------------
Ship it!
Ship It!
- Isabel Jimenez
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
>
>
Re: Review Request 36036: Add `version` string to MasterInfo
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36036/#review89871
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36036]
All tests passed.
- Mesos ReviewBot
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
>
>