----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26701/#review57285 -----------------------------------------------------------
Ship it! Code looks good, thanks! Mostly left comments around better comments, and some minor code movement. Curious if we can pick a better name for `unacknowledged_state` in `Task`: `status_update_state` `status_update_uuid` `current_status_update_state` `current_status_update_uuid` `latest_status_update_state` `latest_status_update_uuid` src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97845> Isn't there a bit more to this comment? If this is a 0.21.0 master and there are 0.20.0 slaves, this all makes sense (the 0.20.0 slaves will not send this field when re-registering so we won't have it here as you said). However, if this is a 0.21.0 master and there are 0.21.0 slaves, then the state would have been set when the slave re-registered with the `Task`, right..? We never unset the unacknowledged state when an acknowledgement occurs on the slave either. "unacknowledged" now seems a bit confusing. Let's chat on Monday so I can understand this a bit better :) src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97846> Hopefully in clarifying my comment above we can be a little more specific in this warning message. src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97847> Could have the following above: // Must be set or unset together. CHECK(task->has_unacknoweldged_uuid() == task->has_unacknowledged_state()); src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97848> Maybe warrants a comment: // We remove the task once a terminal update has // been acknowledged by the framework. src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97850> Why not move this down to where it's used? src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97849> I'm a bit worried about this TODO because it's too inquisitive, someone coming along here doesn't have any context and might think we can remove this. It should not be possible, but to protect against bugs here a CHECK is probably too severe, which means we'll continue to log an error and guard it, in which case we should just update the comment as a NOTE about this. Are you thinking of other alternatives? src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97851> Can you set the state in the similar if / else above, any reason to do the same check twice? src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97852> Seems nice to move this up as well (when you move `set_state` up). src/master/master.cpp <https://reviews.apache.org/r/26701/#comment97853> Looks like this can be wrapped at the LOG(INFO) << line while still being under 80 characters: len(' LOG(INFO) << "Updating the latest state of task " << task->task_id()') len(' << " of framework " << task->framework_id()') len(' << " to " << task->state()') len(' << (task->state() != status.state()') len(' ? " (unacknowledged state: " + stringify(status.state()) + ")"') len(' : "");') Longest line is 78 characters..? src/tests/master_tests.cpp <https://reviews.apache.org/r/26701/#comment97854> Quite the long sentence ;) src/tests/master_tests.cpp <https://reviews.apache.org/r/26701/#comment97855> We should probably encode the release of resources in the name? ReleaseResourcesForTerminalTaskWithPendingUpdates - Ben Mahler On Oct. 17, 2014, 12:31 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26701/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 12:31 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 > ------- > > Master now maintains the latest and unacknowledged states of the task. > > > Diffs > ----- > > src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 > src/master/master.cpp 0a5c9a374062a241c90ea238725fbb8dd2408ef4 > src/tests/master_tests.cpp d9dc40c6f5aaa66e1f7a0e1b7e4d9cdc586ca0fd > > Diff: https://reviews.apache.org/r/26701/diff/ > > > Testing > ------- > > make check > > Ran the new test 1000 times. > > > Thanks, > > Vinod Kone > >
