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 19:49:19 GMT


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


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