You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/07/24 03:45:09 UTC
Review Request 36760: Handle scheduler pid as optional in the slave.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36760/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Vinod Kone.
Bugs: MESOS-3132
https://issues.apache.org/jira/browse/MESOS-3132
Repository: mesos
Description
-------
This is anticipation of HTTP scheduler support in 0.24.0.
Note that the 'pid' is set for driver-based schedulers. The
corresponding master changes to not set 'pid' for HTTP
schedulers have not occurred yet.
Diffs
-----
src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
src/slave/slave.cpp dc12c45516ab39d74a5c29b657f22f74d0acf24e
src/slave/state.hpp 4e00468a777145e3c61b8dee7dfe496f8d65b0e4
src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a
src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9
src/tests/slave_tests.cpp e1390ad84b0003052681600deb9ca518defc0970
Diff: https://reviews.apache.org/r/36760/diff/
Testing
-------
Added tests in [r/36761](https://reviews.apache.org/r/36761/).
Thanks,
Ben Mahler
Re: Review Request 36760: Handle scheduler pid as optional in the
slave.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36760/#review93001
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On July 24, 2015, 10:27 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36760/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 10:27 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-3132
> https://issues.apache.org/jira/browse/MESOS-3132
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is anticipation of HTTP scheduler support in 0.24.0.
> Note that the 'pid' is set for driver-based schedulers. The
> corresponding master changes to not set 'pid' for HTTP
> schedulers have not occurred yet.
>
>
> Diffs
> -----
>
> src/master/master.cpp 7796630a93705bd62157e98e1e4855c68ea7cd0a
> src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp dc12c45516ab39d74a5c29b657f22f74d0acf24e
> src/slave/state.hpp 4e00468a777145e3c61b8dee7dfe496f8d65b0e4
> src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a
> src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9
> src/tests/slave_tests.cpp b145d76cf9e96fe89a5e3df1c4611a6b146585e9
>
> Diff: https://reviews.apache.org/r/36760/diff/
>
>
> Testing
> -------
>
> Added tests in [r/36761](https://reviews.apache.org/r/36761/).
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 36760: Handle scheduler pid as optional in the
slave.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36760/
-----------------------------------------------------------
(Updated July 24, 2015, 10:27 p.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Changes
-------
Cleanups from reviews.
Bugs: MESOS-3132
https://issues.apache.org/jira/browse/MESOS-3132
Repository: mesos
Description
-------
This is anticipation of HTTP scheduler support in 0.24.0.
Note that the 'pid' is set for driver-based schedulers. The
corresponding master changes to not set 'pid' for HTTP
schedulers have not occurred yet.
Diffs (updated)
-----
src/master/master.cpp 7796630a93705bd62157e98e1e4855c68ea7cd0a
src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
src/slave/slave.cpp dc12c45516ab39d74a5c29b657f22f74d0acf24e
src/slave/state.hpp 4e00468a777145e3c61b8dee7dfe496f8d65b0e4
src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a
src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9
src/tests/slave_tests.cpp b145d76cf9e96fe89a5e3df1c4611a6b146585e9
Diff: https://reviews.apache.org/r/36760/diff/
Testing
-------
Added tests in [r/36761](https://reviews.apache.org/r/36761/).
Thanks,
Ben Mahler
Re: Review Request 36760: Handle scheduler pid as optional in the
slave.
Posted by Ben Mahler <be...@gmail.com>.
> On July 24, 2015, 2:17 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp, line 1302
> > <https://reviews.apache.org/r/36760/diff/1/?file=1020568#file1020568line1302>
> >
> > Option<UPID frameworkPid;
> >
> > The default constructor already initializes to None.
Right, we often do this just to make it more readable, although I can see how that's a bit inconsistent with our other types.. hmm..
> On July 24, 2015, 2:17 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp, line 1758
> > <https://reviews.apache.org/r/36760/diff/1/?file=1020568#file1020568line1758>
> >
> > Curious, why not UPID() as is the case at other places ?
Thanks, I'll use getOrElse then.
> On July 24, 2015, 2:17 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp, line 3039
> > <https://reviews.apache.org/r/36760/diff/1/?file=1020568#file1020568line3039>
> >
> > s/to/at
This is where the message is being sent "to", we use "from" and "to" pretty consistently, why "at"?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36760/#review92855
-----------------------------------------------------------
On July 24, 2015, 1:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36760/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 1:45 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-3132
> https://issues.apache.org/jira/browse/MESOS-3132
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is anticipation of HTTP scheduler support in 0.24.0.
> Note that the 'pid' is set for driver-based schedulers. The
> corresponding master changes to not set 'pid' for HTTP
> schedulers have not occurred yet.
>
>
> Diffs
> -----
>
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
> src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp dc12c45516ab39d74a5c29b657f22f74d0acf24e
> src/slave/state.hpp 4e00468a777145e3c61b8dee7dfe496f8d65b0e4
> src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a
> src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9
> src/tests/slave_tests.cpp e1390ad84b0003052681600deb9ca518defc0970
>
> Diff: https://reviews.apache.org/r/36760/diff/
>
>
> Testing
> -------
>
> Added tests in [r/36761](https://reviews.apache.org/r/36761/).
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 36760: Handle scheduler pid as optional in the
slave.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36760/#review92855
-----------------------------------------------------------
Looks very good !
src/messages/messages.proto (line 352)
<https://reviews.apache.org/r/36760/#comment147108>
s/required/optional
src/slave/slave.cpp (line 1299)
<https://reviews.apache.org/r/36760/#comment147109>
Option<UPID frameworkPid;
The default constructor already initializes to None.
src/slave/slave.cpp (line 1753)
<https://reviews.apache.org/r/36760/#comment147110>
Curious, why not UPID() as is the case at other places ?
src/slave/slave.cpp (line 3030)
<https://reviews.apache.org/r/36760/#comment147113>
s/to/at
src/slave/state.hpp (line 253)
<https://reviews.apache.org/r/36760/#comment147111>
s/do not have a a/do not have a
- Anand Mazumdar
On July 24, 2015, 1:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36760/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 1:45 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-3132
> https://issues.apache.org/jira/browse/MESOS-3132
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is anticipation of HTTP scheduler support in 0.24.0.
> Note that the 'pid' is set for driver-based schedulers. The
> corresponding master changes to not set 'pid' for HTTP
> schedulers have not occurred yet.
>
>
> Diffs
> -----
>
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
> src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp dc12c45516ab39d74a5c29b657f22f74d0acf24e
> src/slave/state.hpp 4e00468a777145e3c61b8dee7dfe496f8d65b0e4
> src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a
> src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9
> src/tests/slave_tests.cpp e1390ad84b0003052681600deb9ca518defc0970
>
> Diff: https://reviews.apache.org/r/36760/diff/
>
>
> Testing
> -------
>
> Added tests in [r/36761](https://reviews.apache.org/r/36761/).
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 36760: Handle scheduler pid as optional in the
slave.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36760/#review92945
-----------------------------------------------------------
modulo anand's comments.
src/messages/messages.proto (lines 347 - 351)
<https://reviews.apache.org/r/36760/#comment147204>
I would recommend just pointing to RunTaskMessage.pid for comments to avoid duplication.
src/slave/slave.cpp (lines 2145 - 2146)
<https://reviews.apache.org/r/36760/#comment147211>
Do you want to expand on the comment, on why we do this for posterity?
src/slave/slave.cpp (line 4733)
<https://reviews.apache.org/r/36760/#comment147212>
new line, like you did above?
- Vinod Kone
On July 24, 2015, 1:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36760/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 1:45 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-3132
> https://issues.apache.org/jira/browse/MESOS-3132
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is anticipation of HTTP scheduler support in 0.24.0.
> Note that the 'pid' is set for driver-based schedulers. The
> corresponding master changes to not set 'pid' for HTTP
> schedulers have not occurred yet.
>
>
> Diffs
> -----
>
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
> src/messages/messages.proto 165a16d91616b75c839c7fbf188dc49f029a8c45
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp dc12c45516ab39d74a5c29b657f22f74d0acf24e
> src/slave/state.hpp 4e00468a777145e3c61b8dee7dfe496f8d65b0e4
> src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a
> src/tests/mesos.cpp f09ef0f99573716de8905f49dcc0c9df20e31ea9
> src/tests/slave_tests.cpp e1390ad84b0003052681600deb9ca518defc0970
>
> Diff: https://reviews.apache.org/r/36760/diff/
>
>
> Testing
> -------
>
> Added tests in [r/36761](https://reviews.apache.org/r/36761/).
>
>
> Thanks,
>
> Ben Mahler
>
>