----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10438/#review19138 -----------------------------------------------------------
src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39685> What happened here? src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39686> Why not? Seems like frameworks would want a LOST update when launching a task and we can't launch it. The alternative to me seems to be to wait for the framework to terminate first before launching further tasks. Shouldn't you do one of these options? src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39687> Based on this comment, seems like we should be sending LOST when we can't find the framework as well, how are these different cases? src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39688> Ditto to my earlier question. src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39689> Again, we send the update when we can't find the executor, but we don't send one when we can't find the framework? src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39690> Any CHECKs like this should print the state so that we can see it when it fails: CHECK(....) << executor->state; Which makes me think we should explicitly number our enum members. src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39691> Seems like this case can wrap on the paren: dispatch(isolator, &Isolator::resourcesChanged, ... src/slave/slave.cpp <https://reviews.apache.org/r/10438/#comment39692> One confusing thing here, is that I look at the signatures update(Update, SlaveID, ExecutorID, UUID) update(Update, SlaveID) From your comments, the first one checkpoints and reliably sends the update. The second just retries the update? This seems unintuitive, or am I missing something here? src/slave/status_update_manager.hpp <https://reviews.apache.org/r/10438/#comment39693> Whether sounds like a boolean return value. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/10438/#comment39695> So.. given my earlier comment, why isn't this called retry? src/slave/status_update_manager.hpp <https://reviews.apache.org/r/10438/#comment39694> Ditto. src/slave/status_update_manager.cpp <https://reviews.apache.org/r/10438/#comment39696> s/checkpoint status/checkpoint value/? - Ben Mahler On April 12, 2013, 9:27 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10438/ > ----------------------------------------------------------- > > (Updated April 12, 2013, 9:27 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > This was split off https://reviews.apache.org/r/10142/. > > While I simplified statusUpdate() and StatusUpdateManager a lot, this diff > introduced quite a few changes. > > So, I will leave it up to you, to take another look. > > > Diffs > ----- > > src/slave/slave.cpp 325231458a6883019436e7cc5a37f85f0f5735fa > src/slave/status_update_manager.hpp > e6ca40c5c05c0952cf76fb1db7eff2e4270c0d24 > src/slave/status_update_manager.cpp > 044d245f370ef23ddc67fadbf7f8fe9d75dd662a > > Diff: https://reviews.apache.org/r/10438/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
