You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/02/02 06:12:37 UTC
Review Request 43075: Fixed error message when dropping a
scheduler::Call message.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Repository: mesos
Description
-------
Old output: "Dropping 2 call from framework ..."
New output: "Dropping TEARDOWN call from framework ..."
Diffs
-----
src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
Diff: https://reviews.apache.org/r/43075/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 43075: Fixed definition of operator<< for
`Call::Type` and `Event::Type`.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/#review117484
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Feb. 2, 2016, 9:33 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 9:33 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The types were defined in the `mesos::scheduler` namespace, but the
> overload of `operator<<` was only defined in the `mesos` namespace.
> Because of how ADL works, this usually resulted in the overload
> not being found.
>
> Old output: "Dropping 2 call from framework ..."
>
> New output: "Dropping TEARDOWN call from framework ..."
>
>
> Diffs
> -----
>
> include/mesos/scheduler/scheduler.hpp cadd7df11bb6d4210c5abdab5c04586e6686af1d
> src/sched/sched.cpp 8e51752536041b896e42d44e6e9eae909b0a1d68
>
> Diff: https://reviews.apache.org/r/43075/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 43075: Fixed definition of operator<< for
`Call::Type` and `Event::Type`.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/
-----------------------------------------------------------
(Updated Feb. 2, 2016, 9:36 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Minor fixes: add explicit header include, make style consistent with v1 version
Repository: mesos
Description
-------
The types were defined in the `mesos::scheduler` namespace, but the
overload of `operator<<` was only defined in the `mesos` namespace.
Because of how ADL works, this usually resulted in the overload
not being found.
Old output: "Dropping 2 call from framework ..."
New output: "Dropping TEARDOWN call from framework ..."
Diffs (updated)
-----
include/mesos/scheduler/scheduler.hpp cadd7df11bb6d4210c5abdab5c04586e6686af1d
src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
src/sched/sched.cpp 8e51752536041b896e42d44e6e9eae909b0a1d68
Diff: https://reviews.apache.org/r/43075/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 43075: Fixed definition of operator<< for
`Call::Type` and `Event::Type`.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/
-----------------------------------------------------------
(Updated Feb. 2, 2016, 9:33 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
New approach, per discussion with Vinod.
Summary (updated)
-----------------
Fixed definition of operator<< for `Call::Type` and `Event::Type`.
Repository: mesos
Description (updated)
-------
The types were defined in the `mesos::scheduler` namespace, but the
overload of `operator<<` was only defined in the `mesos` namespace.
Because of how ADL works, this usually resulted in the overload
not being found.
Old output: "Dropping 2 call from framework ..."
New output: "Dropping TEARDOWN call from framework ..."
Diffs (updated)
-----
include/mesos/scheduler/scheduler.hpp cadd7df11bb6d4210c5abdab5c04586e6686af1d
src/sched/sched.cpp 8e51752536041b896e42d44e6e9eae909b0a1d68
Diff: https://reviews.apache.org/r/43075/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 43075: Fixed error message when dropping a
scheduler::Call message.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/#review117370
-----------------------------------------------------------
I saw that there are still other places using the call.type() directly, can you please update those files as well?
Such as the executor, the scheduler library etc.
- Guangya Liu
On 二月 2, 2016, 5:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> -----------------------------------------------------------
>
> (Updated 二月 2, 2016, 5:12 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Old output: "Dropping 2 call from framework ..."
>
> New output: "Dropping TEARDOWN call from framework ..."
>
>
> Diffs
> -----
>
> src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
>
> Diff: https://reviews.apache.org/r/43075/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 43075: Fixed error message when dropping a
scheduler::Call message.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/#review117363
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On 二月 2, 2016, 5:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> -----------------------------------------------------------
>
> (Updated 二月 2, 2016, 5:12 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Old output: "Dropping 2 call from framework ..."
>
> New output: "Dropping TEARDOWN call from framework ..."
>
>
> Diffs
> -----
>
> src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
>
> Diff: https://reviews.apache.org/r/43075/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 43075: Fixed error message when dropping a
scheduler::Call message.
Posted by Neil Conway <ne...@gmail.com>.
> On Feb. 2, 2016, 7:54 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1801
> > <https://reviews.apache.org/r/43075/diff/1/?file=1228851#file1228851line1801>
> >
> > The output stream operator overload for Call (unversioned) defined in "mesos/scheduler/scheduler.hpp". Not sure why the compiler unable to find that overload (defined in `mesos` namespace).
> >
> > It seems to work fine for log statements (that print v1::Call) in scheduler.cpp. The overload for v1::Call was defined in "mesos/include/mesos/v1/scheduler/scheduler.cpp" though, which is in the same namespace as the log statements in scheduler.cpp.
I _believe_ what is going on here is that the overload of `operator<<` for `mesos::scheduler::Call::Type` is defined in the `mesos` namespace, not the `mesos::scheduler` namespace. Since ADL only considers the "closest enclosing namespace" of the argument (see http://en.cppreference.com/w/cpp/language/adl , case 2(d)), it looks for an overload in `mesos::scheduler` but not `mesos`.
Defining the overload of `operator<<` in `mesos::scheduler` seems to fix the problem. I think it is generally good practice to group types and functions on those types together into the same namespace, anyway. I'll submit a revised RR doing that shortly.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/#review117459
-----------------------------------------------------------
On Feb. 2, 2016, 5:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 5:12 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Old output: "Dropping 2 call from framework ..."
>
> New output: "Dropping TEARDOWN call from framework ..."
>
>
> Diffs
> -----
>
> src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
>
> Diff: https://reviews.apache.org/r/43075/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 43075: Fixed error message when dropping a
scheduler::Call message.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 2, 2016, 7:54 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1801
> > <https://reviews.apache.org/r/43075/diff/1/?file=1228851#file1228851line1801>
> >
> > The output stream operator overload for Call (unversioned) defined in "mesos/scheduler/scheduler.hpp". Not sure why the compiler unable to find that overload (defined in `mesos` namespace).
> >
> > It seems to work fine for log statements (that print v1::Call) in scheduler.cpp. The overload for v1::Call was defined in "mesos/include/mesos/v1/scheduler/scheduler.cpp" though, which is in the same namespace as the log statements in scheduler.cpp.
>
> Neil Conway wrote:
> I _believe_ what is going on here is that the overload of `operator<<` for `mesos::scheduler::Call::Type` is defined in the `mesos` namespace, not the `mesos::scheduler` namespace. Since ADL only considers the "closest enclosing namespace" of the argument (see http://en.cppreference.com/w/cpp/language/adl , case 2(d)), it looks for an overload in `mesos::scheduler` but not `mesos`.
>
> Defining the overload of `operator<<` in `mesos::scheduler` seems to fix the problem. I think it is generally good practice to group types and functions on those types together into the same namespace, anyway. I'll submit a revised RR doing that shortly.
Yup, that's what I figured out after reading SO. Just doing a `make check` to confirm.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/#review117459
-----------------------------------------------------------
On Feb. 2, 2016, 5:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 5:12 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Old output: "Dropping 2 call from framework ..."
>
> New output: "Dropping TEARDOWN call from framework ..."
>
>
> Diffs
> -----
>
> src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
>
> Diff: https://reviews.apache.org/r/43075/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 43075: Fixed error message when dropping a
scheduler::Call message.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/#review117459
-----------------------------------------------------------
src/master/master.cpp (line 1801)
<https://reviews.apache.org/r/43075/#comment178649>
The output stream operator overload for Call (unversioned) defined in "mesos/scheduler/scheduler.hpp". Not sure why the compiler unable to find that overload (defined in `mesos` namespace).
It seems to work fine for log statements (that print v1::Call) in scheduler.cpp. The overload for v1::Call was defined in "mesos/include/mesos/v1/scheduler/scheduler.cpp" though, which is in the same namespace as the log statements in scheduler.cpp.
- Vinod Kone
On Feb. 2, 2016, 5:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 5:12 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Old output: "Dropping 2 call from framework ..."
>
> New output: "Dropping TEARDOWN call from framework ..."
>
>
> Diffs
> -----
>
> src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
>
> Diff: https://reviews.apache.org/r/43075/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 43075: Fixed error message when dropping a
scheduler::Call message.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43075/#review117369
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43075]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 2, 2016, 5:12 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 5:12 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Old output: "Dropping 2 call from framework ..."
>
> New output: "Dropping TEARDOWN call from framework ..."
>
>
> Diffs
> -----
>
> src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83
>
> Diff: https://reviews.apache.org/r/43075/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>