You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2014/10/01 21:15:43 UTC
Re: Review Request 26159: Fixed framework logging in master.cpp.
-----------------------------------------------------------
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
>
>
Re: Review Request 26159: Fixed framework logging in master.cpp.
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 1, 2014, 7:15 p.m., Ben Mahler wrote:
> > 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.
Regarding wrapping, there is no consistency in this file (and the code base) around wrapping log lines :/ I tried to wrap them with some subjective measure of readability (e.g., '"..framework " << *framework' is better than '"..framework" \n *framework') in mind. We can revisit this later if we can figure out some rules.
> On Oct. 1, 2014, 7:15 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3425-3426
> > <https://reviews.apache.org/r/26159/diff/1/?file=708980#file708980line3425>
> >
> > You don't want logging of the expected pid anymore?
forgot to s/frameworkId/*framework/. "*framework" should print the expected pid.
> On Oct. 1, 2014, 7:15 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 3933-3936
> > <https://reviews.apache.org/r/26159/diff/1/?file=708980#file708980line3933>
> >
> > Wonder why the "Slave" line is so short.. and maybe we want to avoid the period in favor of a semi-colon here?
reformatted to two lines. period seems fine to me like we did on #3905?
> On Oct. 1, 2014, 7:15 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1362-1364
> > <https://reviews.apache.org/r/26159/diff/1/?file=708980#file708980line1362>
> >
> > 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.
i used quotes for cases when there is no framework id, because it reads better.
example:
"dropping request for framework 'foobar'" is better than "dropping request for framework (foobar)".
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26159/#review55114
-----------------------------------------------------------
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
>
>