----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26699/#review57185 -----------------------------------------------------------
Looks good, mainly just comments about adding more comments for myself and for posterity. :) src/messages/messages.proto <https://reviews.apache.org/r/26699/#comment97711> Wonder if it's time for a UUID wrapper message type akin to what we did with all of our _ID types.. Just putting it out there as potential TODO material. :) src/slave/slave.cpp <https://reviews.apache.org/r/26699/#comment97715> I intentionally didn't include newlines here because I wanted to capture this as one small logical block of code (add all the tasks) and newlines were not used below for completed tasks either (granted that one does look less readable) :) src/slave/slave.cpp <https://reviews.apache.org/r/26699/#comment97718> Hm.. could this note expand on how it's possible for them not to exist? Needs some non-local reasoning? Is it due to recovery? src/slave/slave.cpp <https://reviews.apache.org/r/26699/#comment97721> Maybe a newline after this? A bit dense IMO :) src/slave/slave.cpp <https://reviews.apache.org/r/26699/#comment97722> Could you add a little comment about where we're looking here? In particular, why queued tasks and completed tasks are ignored. Or, should there be a CHECK for no queued task matching this? src/slave/slave.cpp <https://reviews.apache.org/r/26699/#comment97720> Since the goal of this block is to set the unacked state, could this comment be moved above the note? The note threw me off a bit because I initially associated it with the _goal_ of this block of code, which is actually just to set the unacked state. The other important thing to comment on is why we're setting the state here as opposed to when we first receive the update, for posterity. Seems really non-obvious to me. src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/26699/#comment97717> How did this get in here? ;) - Ben Mahler On Oct. 17, 2014, 12:26 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26699/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 12:26 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 > ------- > > Slave re-registration now sends both the latest state and unacknowledged > state to the master. > > > Diffs > ----- > > src/messages/messages.proto 8de7f9699f43aa2780f4a39fed087abcf5e5af99 > src/slave/slave.cpp 0e342ed35e3db3b68f9f32b6cf4ace23e4a4db38 > src/tests/fault_tolerance_tests.cpp > a75910d4f486230ba3f1d8927e5f1e5fda6e287b > src/tests/slave_tests.cpp f585bdd20ae1af466f2c1b4d85331ac67451552f > > Diff: https://reviews.apache.org/r/26699/diff/ > > > Testing > ------- > > make check > > Ran new test 1000 times. > > > Thanks, > > Vinod Kone > >
