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/21 01:51:09 UTC
Review Request 26957: Added pause() and resume() to status update
manager.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26957/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Renamed flush() to resume() and added pause() to status update manager. pause() should let status update manager know to not send updates when slave is disconnected.
Also updated the constructor of status update manager.
Diffs
-----
src/local/local.cpp 66de798377269150a2e546daa5ca7c5371bbce79
src/slave/main.cpp b27cc32ebccb1c97f2f2ae0b904c725bbf541ebf
src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
src/slave/status_update_manager.hpp c371e5542e12cd6bb9401591cd691d99bc0fa8fd
src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
src/tests/cluster.hpp ee194ad0b0014eb4c415c294cfeaf5f7f3089a7f
src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992
src/tests/mesos.cpp 147e23fec94add19f9d5c34e6f7e97874db9b6b2
Diff: https://reviews.apache.org/r/26957/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 26957: Added pause() and resume() to status update
manager.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26957/
-----------------------------------------------------------
(Updated Oct. 21, 2014, 10:22 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
benm's comments. NNFR.
Repository: mesos-git
Description
-------
Renamed flush() to resume() and added pause() to status update manager. pause() should let status update manager know to not send updates when slave is disconnected.
Also updated the constructor of status update manager.
Diffs (updated)
-----
src/local/local.cpp 66de798377269150a2e546daa5ca7c5371bbce79
src/slave/main.cpp b27cc32ebccb1c97f2f2ae0b904c725bbf541ebf
src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
src/slave/status_update_manager.hpp c371e5542e12cd6bb9401591cd691d99bc0fa8fd
src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
src/tests/cluster.hpp ee194ad0b0014eb4c415c294cfeaf5f7f3089a7f
src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992
src/tests/mesos.cpp 147e23fec94add19f9d5c34e6f7e97874db9b6b2
Diff: https://reviews.apache.org/r/26957/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 26957: Added pause() and resume() to status update
manager.
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 21, 2014, 6:27 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.cpp, lines 171-175
> > <https://reviews.apache.org/r/26957/diff/1/?file=726661#file726661line171>
> >
> > Why doesn't pause() clear all the stream timeouts by setting them to None()?
> >
> > Then the timeout() method wouldn't have to explicitly look at 'paused', since no timers are expired.
clearing "timeout" seems a bit weird because "timeout" tells whether there is a pending timeout, not whether the SUM is paused. overloading the semantics will be weird.
i think the main issue is that forward() returns expired Timeout(). i'll change it so that the callers don't call forward() is SUM is paused.
this is more explicit.
> On Oct. 21, 2014, 6:27 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.cpp, line 371
> > <https://reviews.apache.org/r/26957/diff/1/?file=726661#file726661line371>
> >
> > Seems like this should return a None(), so that the stream->timeout is None() instead of an expired Timeout. Then forward returns a None() timeout when paused.
> >
> > Or if you don't want to change the signature of forward, then we could only call forward when we're not paused, which is more explicit but trickier for callers.
> >
> > Also, why the one-line bracing like this?
see above.
> On Oct. 21, 2014, 6:27 p.m., Ben Mahler wrote:
> > src/slave/status_update_manager.cpp, line 469
> > <https://reviews.apache.org/r/26957/diff/1/?file=726661#file726661line469>
> >
> > If we set the Timeouts to None() when pausing, we can just remove this CHECK and consider None() timers as not expired.
see above.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26957/#review57517
-----------------------------------------------------------
On Oct. 20, 2014, 11:51 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26957/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 11:51 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Renamed flush() to resume() and added pause() to status update manager. pause() should let status update manager know to not send updates when slave is disconnected.
>
> Also updated the constructor of status update manager.
>
>
> Diffs
> -----
>
> src/local/local.cpp 66de798377269150a2e546daa5ca7c5371bbce79
> src/slave/main.cpp b27cc32ebccb1c97f2f2ae0b904c725bbf541ebf
> src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
> src/slave/status_update_manager.hpp c371e5542e12cd6bb9401591cd691d99bc0fa8fd
> src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
> src/tests/cluster.hpp ee194ad0b0014eb4c415c294cfeaf5f7f3089a7f
> src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992
> src/tests/mesos.cpp 147e23fec94add19f9d5c34e6f7e97874db9b6b2
>
> Diff: https://reviews.apache.org/r/26957/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 26957: Added pause() and resume() to status update
manager.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26957/#review57517
-----------------------------------------------------------
Ship it!
Looks good, although I'm thinking we should be setting the stream timeouts to None() when we're paused, rather than an expired timeout.
src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/26957/#comment98239>
Hm.. instead of including <functional> and using std::function, can you instead include <stout/lambda.hpp> and use lambda::function?
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/26957/#comment98419>
Why doesn't pause() clear all the stream timeouts by setting them to None()?
Then the timeout() method wouldn't have to explicitly look at 'paused', since no timers are expired.
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/26957/#comment98416>
Seems like this should return a None(), so that the stream->timeout is None() instead of an expired Timeout. Then forward returns a None() timeout when paused.
Or if you don't want to change the signature of forward, then we could only call forward when we're not paused, which is more explicit but trickier for callers.
Also, why the one-line bracing like this?
src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/26957/#comment98420>
If we set the Timeouts to None() when pausing, we can just remove this CHECK and consider None() timers as not expired.
- Ben Mahler
On Oct. 20, 2014, 11:51 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26957/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2014, 11:51 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Renamed flush() to resume() and added pause() to status update manager. pause() should let status update manager know to not send updates when slave is disconnected.
>
> Also updated the constructor of status update manager.
>
>
> Diffs
> -----
>
> src/local/local.cpp 66de798377269150a2e546daa5ca7c5371bbce79
> src/slave/main.cpp b27cc32ebccb1c97f2f2ae0b904c725bbf541ebf
> src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b
> src/slave/status_update_manager.hpp c371e5542e12cd6bb9401591cd691d99bc0fa8fd
> src/slave/status_update_manager.cpp 5d5cf234ef2dd47fa4b1f67be761dbca31659451
> src/tests/cluster.hpp ee194ad0b0014eb4c415c294cfeaf5f7f3089a7f
> src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992
> src/tests/mesos.cpp 147e23fec94add19f9d5c34e6f7e97874db9b6b2
>
> Diff: https://reviews.apache.org/r/26957/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>