You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2014/10/17 02:30:41 UTC
Re: Review Request 26700: Updated Slave to send latest task state in
update.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/
-----------------------------------------------------------
(Updated Oct. 17, 2014, 12:30 a.m.)
Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
Changes
-------
Per new design, the latest state is now set by the slave instead of the status update manager.
Summary (updated)
-----------------
Updated Slave to send latest task state in update.
Bugs: MESOS-1799 and MESOS-1817
https://issues.apache.org/jira/browse/MESOS-1799
https://issues.apache.org/jira/browse/MESOS-1817
Repository: mesos-git
Description
-------
Status update manager now sends both latest and unacknowledged states to the master.
Diffs (updated)
-----
src/messages/messages.proto 8de7f9699f43aa2780f4a39fed087abcf5e5af99
src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38
src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
Diff: https://reviews.apache.org/r/26700/diff/
Testing
-------
make check
Ran new test 1000 times.
Thanks,
Vinod Kone
Re: Review Request 26700: Updated Slave to send latest task state in
update.
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 17, 2014, 9:30 p.m., Ben Mahler wrote:
> > src/messages/messages.proto, lines 76-78
> > <https://reviews.apache.org/r/26700/diff/3/?file=723861#file723861line76>
> >
> > Looks like you wrote this comment before it was moved down to the variable, no need to write 'latest_state' anymore right?
> >
> > Some more substance to this comment would be nice for posterity, i.e. what is it that makes this state different from the one in TaskStatus?
done.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/#review57219
-----------------------------------------------------------
On Oct. 17, 2014, 12:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26700/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 12:30 a.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
>
>
> Bugs: MESOS-1799 and MESOS-1817
> https://issues.apache.org/jira/browse/MESOS-1799
> https://issues.apache.org/jira/browse/MESOS-1817
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Status update manager now sends both latest and unacknowledged states to the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 8de7f9699f43aa2780f4a39fed087abcf5e5af99
> src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38
> src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
>
> Diff: https://reviews.apache.org/r/26700/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran new test 1000 times.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 26700: Updated Slave to send latest task state in
update.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/#review57219
-----------------------------------------------------------
Ship it!
src/messages/messages.proto
<https://reviews.apache.org/r/26700/#comment97758>
Looks like you wrote this comment before it was moved down to the variable, no need to write 'latest_state' anymore right?
Some more substance to this comment would be nice for posterity, i.e. what is it that makes this state different from the one in TaskStatus?
src/slave/slave.cpp
<https://reviews.apache.org/r/26700/#comment97765>
This ties into my comment from an earlier review, for posterity it would be great to mention why 'latest' is driven by the slave's receipt of updates from executors, and 'unacknowledged' is driven by the receipt of updates from the SUM (as opposed to acks from master).
In particular, we should probably call out that this might be stale due to the race (ack received by slave but in flight to SUM), and why that is safe.
- Ben Mahler
On Oct. 17, 2014, 12:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26700/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 12:30 a.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
>
>
> Bugs: MESOS-1799 and MESOS-1817
> https://issues.apache.org/jira/browse/MESOS-1799
> https://issues.apache.org/jira/browse/MESOS-1817
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Status update manager now sends both latest and unacknowledged states to the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 8de7f9699f43aa2780f4a39fed087abcf5e5af99
> src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38
> src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
>
> Diff: https://reviews.apache.org/r/26700/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran new test 1000 times.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 26700: Updated Slave to send latest task state in
update.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/
-----------------------------------------------------------
(Updated Oct. 21, 2014, 10:47 p.m.)
Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
Changes
-------
changed the signature of Slave::forward() to take non-const param per jie's comment. NNFR.
Bugs: MESOS-1799 and MESOS-1817
https://issues.apache.org/jira/browse/MESOS-1799
https://issues.apache.org/jira/browse/MESOS-1817
Repository: mesos-git
Description
-------
Status update manager now sends both latest and unacknowledged states to the master.
Diffs (updated)
-----
src/messages/messages.proto 0dfc1b78dcf728bbf2b6094fb6111400c7af7362
src/slave/slave.hpp ccc0e03edbd41d9ce3bb0f6b0447bf4240648986
src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
src/slave/status_update_manager.hpp c371e5542e12cd6bb9401591cd691d99bc0fa8fd
src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
Diff: https://reviews.apache.org/r/26700/diff/
Testing
-------
make check
Ran new test 1000 times.
Thanks,
Vinod Kone
Re: Review Request 26700: Updated Slave to send latest task state in
update.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/
-----------------------------------------------------------
(Updated Oct. 21, 2014, 10:23 p.m.)
Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
Changes
-------
rebased. NNFR.
Bugs: MESOS-1799 and MESOS-1817
https://issues.apache.org/jira/browse/MESOS-1799
https://issues.apache.org/jira/browse/MESOS-1817
Repository: mesos-git
Description
-------
Status update manager now sends both latest and unacknowledged states to the master.
Diffs (updated)
-----
src/messages/messages.proto 0dfc1b78dcf728bbf2b6094fb6111400c7af7362
src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
Diff: https://reviews.apache.org/r/26700/diff/
Testing
-------
make check
Ran new test 1000 times.
Thanks,
Vinod Kone
Re: Review Request 26700: Updated Slave to send latest task state in
update.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/
-----------------------------------------------------------
(Updated Oct. 20, 2014, 11:55 p.m.)
Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
Changes
-------
addressed comments.
Bugs: MESOS-1799 and MESOS-1817
https://issues.apache.org/jira/browse/MESOS-1799
https://issues.apache.org/jira/browse/MESOS-1817
Repository: mesos-git
Description
-------
Status update manager now sends both latest and unacknowledged states to the master.
Diffs (updated)
-----
src/messages/messages.proto 0dfc1b78dcf728bbf2b6094fb6111400c7af7362
src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
Diff: https://reviews.apache.org/r/26700/diff/
Testing
-------
make check
Ran new test 1000 times.
Thanks,
Vinod Kone
Re: Review Request 26700: Updated Slave to send latest task state in
update.
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 17, 2014, 7:25 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 2335-2337
> > <https://reviews.apache.org/r/26700/diff/3/?file=723862#file723862line2335>
> >
> > Use `void Slave::forward(StatusUpdate update)` and avoid the explicit copy.
I typically do/did explicit copy because it is more explicit than changing the argument type to non const. Do we follow a consistent pattern regarding this use case before?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/#review57198
-----------------------------------------------------------
On Oct. 20, 2014, 11:55 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26700/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 11:55 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
>
>
> Bugs: MESOS-1799 and MESOS-1817
> https://issues.apache.org/jira/browse/MESOS-1799
> https://issues.apache.org/jira/browse/MESOS-1817
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Status update manager now sends both latest and unacknowledged states to the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 0dfc1b78dcf728bbf2b6094fb6111400c7af7362
> src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
> src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
>
> Diff: https://reviews.apache.org/r/26700/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran new test 1000 times.
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 26700: Updated Slave to send latest task state in
update.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26700/#review57198
-----------------------------------------------------------
Flying by.
src/slave/slave.cpp
<https://reviews.apache.org/r/26700/#comment97734>
Use `void Slave::forward(StatusUpdate update)` and avoid the explicit copy.
- Jie Yu
On Oct. 17, 2014, 12:30 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26700/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 12:30 a.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen.
>
>
> Bugs: MESOS-1799 and MESOS-1817
> https://issues.apache.org/jira/browse/MESOS-1799
> https://issues.apache.org/jira/browse/MESOS-1817
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Status update manager now sends both latest and unacknowledged states to the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 8de7f9699f43aa2780f4a39fed087abcf5e5af99
> src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38
> src/tests/status_update_manager_tests.cpp e9ef1e208cb01535e9366db7872b922c8f06ec40
>
> Diff: https://reviews.apache.org/r/26700/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran new test 1000 times.
>
>
> Thanks,
>
> Vinod Kone
>
>