> On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: > > Looks great! I inlined a few suggestions below. > > > > Will you be following up with a test?
I may need some help with how to unit test this - but the short answer is **yes!** :) (I wanted this out ASAP, to avoid wasting too much time if I'd misunderstood people's suggestions) > On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: > > src/master/constants.hpp, line 102 > > <https://reviews.apache.org/r/35571/diff/2/?file=986546#file986546line102> > > > > Should we start with 'NOTE:"? > > s/0.24/Mesos 0.24/ Thanks - also below (Mesos 0.23) > On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: > > src/master/detector.cpp, lines 450-473 > > <https://reviews.apache.org/r/35571/diff/2/?file=986548#file986548line450> > > > > A bit dense block - can we split up and comment on the flow? Sure - although, the INFO and WARN messages should give a good sense of what's going on? (also, modeled on the rest of the code in this very same method: in fact, the flow is virtually identical, with the addition of the one-line JSON parsing). Added a few blank lines (note: this is violation of Google's preference for "vertical space frugality") > On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: > > src/master/detector.cpp, lines 456-457 > > <https://reviews.apache.org/r/35571/diff/2/?file=986548#file986548line456> > > > > The wrapping seems off (taken our style guide): > > > > How about: > > ``` > > promises::fail( > > &promises, > > "Failed to parse data into valid JSON: " + > > jsonMasterInfo.error()); > > ``` and a few lines above too. > On June 17, 2015, 9:12 p.m., Niklas Nielsen wrote: > > src/master/detector.cpp, line 468 > > <https://reviews.apache.org/r/35571/diff/2/?file=986548#file986548line468> > > > > Can we inline this in the warning above, so we don't get two log lines? This one is only emitted conditionally, if there is an error. A `Try` (unlike a `Result`), if it's not `some`, can be either `none` or `error`, so we need to cater for both cases. - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88276 ----------------------------------------------------------- On June 17, 2015, 6:18 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35571/ > ----------------------------------------------------------- > > (Updated June 17, 2015, 6:18 p.m.) > > > Review request for mesos, Niklas Nielsen and Vinod Kone. > > > Bugs: MESOS-2340 > https://issues.apache.org/jira/browse/MESOS-2340 > > > Repository: mesos > > > Description > ------- > > Jira: MESOS-2340 > > The Leader detector will now also try to > read nodes with label master::MASTER_INFO_JSON_LABEL > and parse the contents as JSON, converting the data > to a mesos::MasterInfo protobuf. > > The ability to support binary data in PB format is > still supported and the master::Contender will continue > to serialize the binary data, so as to be compatible with > 0.22 Mesos Masters. > > > Diffs > ----- > > src/master/constants.hpp 57cf8fbeff203847b5b5442f6c78ca9c09bcc66d > src/master/constants.cpp 8c7174a9940bd332832bf85d81ab13cf11836dd0 > src/master/detector.cpp 5700711771480f4e5da88e60657618c955f10048 > > Diff: https://reviews.apache.org/r/35571/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
