> On June 18, 2015, 1:08 a.m., Ben Mahler wrote: > > src/master/detector.cpp, lines 444-447 > > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line444> > > > > 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 :) > > Marco Massenzio wrote: > logs are generally meant for parsers and automated tooling (not humans :) > - having multi-line log messages confuses them and is considered to be bad > form in DevOps circles... > > aligned indent and removed closing periods.
I don't follow your point about automated tooling vs. humans. I meant newlines in the code itself, to make it less dense to read the code. > On June 18, 2015, 1:08 a.m., Ben Mahler wrote: > > src/master/detector.cpp, lines 482-483 > > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line482> > > > > 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? > > Marco Massenzio wrote: > There may not be an error: it may simply be None(). Although, I wonder > whether this is even a possibility? > Yet, Try contemplates three possible outcomes which we need to cater for. > > But you are absolutely correct: this was not clean code; modified > similarly to above and changed the log message to make it clearer. Try only has two states: SOME, ERROR. There is no None() case.. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88315 ----------------------------------------------------------- On June 18, 2015, 3:31 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35571/ > ----------------------------------------------------------- > > (Updated June 18, 2015, 3:31 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 > >
