----------------------------------------------------------- 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 > >
