----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26159/#review55114 -----------------------------------------------------------
Ship it! Ah much better. I noticed some of the framework name logging uses single quotes, whereas your standard one uses parens. The LOG(INFO) wrapping on the next line looks weird to me, but it's pretty subjective. src/master/master.cpp <https://reviews.apache.org/r/26159/#comment95476> Why change the wrapping on some of these LOG(INFO) statements? Looks unnecessary in this case? src/master/master.cpp <https://reviews.apache.org/r/26159/#comment95477> Whoops, you indented this one more than the others (4 vs. 2). Isn't this an extra line vs. ``` LOG(INFO) << "Received registration request for framework" << "'" << frameworkInfo.name() << "'" << " at " << from; ``` Seems a bit odd that here you used quotes but in the standard log message you used parens to wrap the framework name, should we stick with one format? Ditto elsewhere below. src/master/master.cpp <https://reviews.apache.org/r/26159/#comment95483> This style wrapping on LOG(INFO) looks a bit strange on the eyes, don't you need 3 lines for the statement either way? FWICT, we tend to wrap LOG(INFO) at the "<<" whereas LOG(WARNING) we sometimes go on the next line because it's a bit longer. (Although I see two exceptions in the master that were introduced as part of the auth code.) src/master/master.cpp <https://reviews.apache.org/r/26159/#comment95484> Ditto here and elsewhere for '' name consistency and the wrapping. src/master/master.cpp <https://reviews.apache.org/r/26159/#comment95485> Looks like this could be wrapped on the LOG(ERROR) line, to become only 2 lines. src/master/master.cpp <https://reviews.apache.org/r/26159/#comment95486> You don't want logging of the expected pid anymore? src/master/master.cpp <https://reviews.apache.org/r/26159/#comment95487> Wonder why the "Slave" line is so short.. and maybe we want to avoid the period in favor of a semi-colon here? - Ben Mahler On Sept. 30, 2014, 1:13 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26159/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2014, 1:13 a.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > Like we did for slave, wanted to standardize how we log about a framework in > master.cpp. Included framework.name() because I think it's more useful for > debugging in a multi-framework world. > > No semantic changes. > > > Diffs > ----- > > src/master/master.hpp d6380199421840aa17d4ce2725dcbcf4a11ce85f > src/master/master.cpp a60308f912a1ed81ecd51c677461a8f591d9eb8e > > Diff: https://reviews.apache.org/r/26159/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
