----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88315 -----------------------------------------------------------
Saw that this got a LGTM, so wanted to help show Nik and Till the kinds of things to think about and to look out for. :) src/master/detector.cpp (lines 431 - 436) <https://reviews.apache.org/r/35571/#comment140782> As a follow up for later, is this case still needed? src/master/detector.cpp (lines 444 - 447) <https://reviews.apache.org/r/35571/#comment140762> Our logging messages do not end with periods, please do a sweep :) Ditto for aligning on the << operator. To be consistent with the rest of our code base. Lastly, normally we would put newlines above and below this to make it less dense for the reader :) src/master/detector.cpp (lines 450 - 451) <https://reviews.apache.org/r/35571/#comment140763> You don't use << on the next line here, but you did above..? Seems inconsistent, let's use << in both cases since that's the general pattern in the code. But, let's just remove this comment entirely, it seems unnecessary and it will show up in 0.23.0 but it says 0.24.0, which may be confusing. src/master/detector.cpp (line 452) <https://reviews.apache.org/r/35571/#comment140761> Can you just inline it in the parse call? Doesn't think this is buying us much. For posterity, we also tend to avoid glueing an assignment right below a statement without a newline, it tends to read too densely. Also, we should avoid 'const' proliferation, for example, 'jsonMasterInfo' and 'masterInfo' can be const here too but that will be pretty dense to read. src/master/detector.cpp (line 454) <https://reviews.apache.org/r/35571/#comment140765> This makes me wonder, what is the "default parser" for JSON, do we have some kind of custom json parser that doesn't follow the RFC..!? :) To me it's pretty obvious that you're parsing the string as JSON, that's what the code shows, so I would remove the comment :) src/master/detector.cpp (line 455) <https://reviews.apache.org/r/35571/#comment140760> (1) s/jsonMasterInfo/object/ or s/jsonMasterInfo/parse/ are common (2) 'auto' here is not that intuitive and looks inconsistent with the rest of our code (e.g. you don't use it in protobuf::parse below). In general, we're trying to use 'auto' where it aids readability, and the type is obvious. Can you remove it? src/master/detector.cpp (lines 455 - 456) <https://reviews.apache.org/r/35571/#comment140769> Mind adding a newline after the variable assignment? We've started to do this in general to improve the readability (less dense as other said). src/master/detector.cpp (lines 464 - 467) <https://reviews.apache.org/r/35571/#comment140766> Are we going to say this every time we convert from JSON to protobuf? Seems excessive, let's just remove the comment. :) src/master/detector.cpp (lines 468 - 470) <https://reviews.apache.org/r/35571/#comment140768> s/masterInfo/info/ to be consistent with the code above :) Mind adding a newline after the variable assignment? We've started to do this in general to improve the readability (less dense as other said). src/master/detector.cpp (lines 470 - 484) <https://reviews.apache.org/r/35571/#comment140776> Any reason not to do the error handling consistently with the json block above? ``` Try<mesos::MasterInfo> info = ...; if (info.isError()) { ... return; } leader = info.get(); ``` src/master/detector.cpp (lines 473 - 475) <https://reviews.apache.org/r/35571/#comment140770> You're already returning the failure to the caller, why are you logging it here as well? The caller with log the failure from the promise. src/master/detector.cpp (lines 482 - 483) <https://reviews.apache.org/r/35571/#comment140771> This is a strange message, why did you say "if any", there is clearly an error here..! Also, why did you need to have two sentences to say this, when one sufficed above? - Ben Mahler On June 17, 2015, 11:40 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, 11:40 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 > >
