You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@apache.org> on 2017/02/10 01:42:21 UTC
Review Request 56524: Added `drop` for overload to avoid custom
logging.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56524/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler and Jay Guo.
Bugs: MESOS-6638
https://issues.apache.org/jira/browse/MESOS-6638
Repository: mesos
Description
-------
Added `drop` for overload to avoid custom logging.
Diffs
-----
src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb
src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c
Diff: https://reviews.apache.org/r/56524/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 56524: Added `drop` for overload to avoid custom
logging.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56524/#review165064
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56499, 56524]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 10, 2017, 1:42 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56524/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 1:42 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added `drop` for overload to avoid custom logging.
>
>
> Diffs
> -----
>
> src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb
> src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c
>
> Diff: https://reviews.apache.org/r/56524/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56524: Added `drop` for overload to avoid custom
logging.
Posted by Guangya Liu <gy...@apache.org>.
> On \u4e8c\u6708 10, 2017, 9:27 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 2246-2247
> > <https://reviews.apache.org/r/56524/diff/1/?file=1629166#file1629166line2246>
> >
> > Can you follow the same format at the drop two lines above?
> >
> > ```
> > LOG(ERROR) << "Dropping " << call.type() << " call"
> > << " from framework " << *framework
> > << ": " << message;
> > ```
> >
> > Not yours, but just as an aside it seems like all of the drop logging should probably be at the warning instead of error level. Feel free to leave as is for now.
Added a `TODO` here to follow up the `WARNING` message.
```
// TODO(gyliu513): Update all `drop` logging to `WARNING`.
LOG(ERROR) << "Dropping " << call.type() << " call"
<< " from framework " << *framework
<< ": " << message;
```
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56524/#review165176
-----------------------------------------------------------
On \u4e8c\u6708 10, 2017, 1:42 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56524/
> -----------------------------------------------------------
>
> (Updated \u4e8c\u6708 10, 2017, 1:42 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added `drop` for overload to avoid custom logging.
>
>
> Diffs
> -----
>
> src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb
> src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c
>
> Diff: https://reviews.apache.org/r/56524/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56524: Added `drop` for overload to avoid custom
logging.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56524/#review165176
-----------------------------------------------------------
src/master/master.cpp (lines 2246 - 2247)
<https://reviews.apache.org/r/56524/#comment236985>
Can you follow the same format at the drop two lines above?
```
LOG(ERROR) << "Dropping " << call.type() << " call"
<< " from framework " << *framework
<< ": " << message;
```
Not yours, but just as an aside it seems like all of the drop logging should probably be at the warning instead of error level. Feel free to leave as is for now.
src/master/master.cpp (lines 3248 - 3253)
<https://reviews.apache.org/r/56524/#comment236986>
I would suggest adding a drop overload for Suppress, so that you can just do:
```
drop(framework,
suppress,
"Suppression role is invalid: " + roleError->message);
```
This could do the `scheduler::Call` construction and call the version you've added in this patch.
As it stands the call-sites need to do the `scheduler::Call` which is error prone. For example, the current code in this patch doesn't fill in the suppress portion of the call:
```
scheduler::Call call;
call.set_type(scheduler::Call::SUPPRESS);
call.mutable_suppress()->CopyFrom(suppress); // This is missed.
```
- Benjamin Mahler
On Feb. 10, 2017, 1:42 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56524/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 1:42 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added `drop` for overload to avoid custom logging.
>
>
> Diffs
> -----
>
> src/master/master.hpp 767ac48f2a65514a19c4a3f8ca7b35f0259f9aeb
> src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c
>
> Diff: https://reviews.apache.org/r/56524/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56524: Added `drop` for overload to avoid custom
logging.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56524/#review165225
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp (line 3278)
<https://reviews.apache.org/r/56524/#comment237054>
-> instead of .get(). ?
src/master/master.cpp (line 4897)
<https://reviews.apache.org/r/56524/#comment237055>
ditto here.
- Benjamin Mahler
On Feb. 11, 2017, 1:02 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56524/
> -----------------------------------------------------------
>
> (Updated Feb. 11, 2017, 1:02 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and Jay Guo.
>
>
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added `drop` for overload to avoid custom logging.
>
>
> Diffs
> -----
>
> src/master/master.hpp 70f50f4264584be55312842b038c925687afb2cb
> src/master/master.cpp 80d481b2ea1435147cd213383008435f35112d92
>
> Diff: https://reviews.apache.org/r/56524/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 56524: Added `drop` for overload to avoid custom
logging.
Posted by Guangya Liu <gy...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56524/
-----------------------------------------------------------
(Updated \u4e8c\u6708 11, 2017, 1:02 a.m.)
Review request for mesos, Benjamin Mahler and Jay Guo.
Bugs: MESOS-6638
https://issues.apache.org/jira/browse/MESOS-6638
Repository: mesos
Description
-------
Added `drop` for overload to avoid custom logging.
Diffs (updated)
-----
src/master/master.hpp 70f50f4264584be55312842b038c925687afb2cb
src/master/master.cpp 80d481b2ea1435147cd213383008435f35112d92
Diff: https://reviews.apache.org/r/56524/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu