> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 431-436
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line431>
> >
> >     As a follow up for later, is this case still needed?

not sure about this - would you like me to file a Jira or add a TODO?


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

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.


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 450-451
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line450>
> >
> >     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.

done


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, line 452
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line452>
> >
> >     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.

the `const` was following a comment from @nnielsen

done


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, line 454
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line454>
> >
> >     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 :)

yes - here, as below, I've added comments following remarks from @nnielsen that 
it was "dense for the reader"
I may have misunderstood what he meant though.

Will remove, thanks (and below too).


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, line 455
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line455>
> >
> >     (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?

er... I figured the `<JSON::Object>` on the other side of the assignment was a 
bit of a giveaway :)

done & renamed too


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 455-456
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line455>
> >
> >     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).

sure


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 464-467
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line464>
> >
> >     Are we going to say this every time we convert from JSON to protobuf? 
> > Seems excessive, let's just remove the comment. :)

I'm with you!
removed


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 470-484
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line470>
> >
> >     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();
> >     ```

yes: this way it groups together both cases where the Try is not Some(): when 
it's None() and when there's an Error().
I'm not sure that this will ever happen (in other words, either it succeeds and 
it's Some() or it fails with an Error()): but this is one part of the code, 
where I'd rather be "belt & braces" than spend hours trying to debug mysterious 
bugs...

I think, this is readable enough as is anyway?


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 473-475
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line473>
> >
> >     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.

old habit, mostly acquired at Google: you always log at the first point of 
failure: makes digging for the "breaking point" a lot easier.


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

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.


> On June 18, 2015, 1:08 a.m., Ben Mahler wrote:
> > src/master/detector.cpp, lines 468-470
> > <https://reviews.apache.org/r/35571/diff/4/?file=986626#file986626line468>
> >
> >     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).

done


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35571/#review88315
-----------------------------------------------------------


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

Reply via email to