> On Oct. 18, 2014, 11:15 p.m., Ben Mahler wrote: > > 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`
picked status_update_state. > On Oct. 18, 2014, 11:15 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2811-2816 > > <https://reviews.apache.org/r/26701/diff/3/?file=723865#file723865line2811> > > > > 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 :) The unacknowledged state of the task will be set by the 0.21.0 *master* when it receives an update from either 0.20.0 or 0.21.0 slave. That is why this comment doesn't talk about the slave versions. > On Oct. 18, 2014, 11:15 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2825-2827 > > <https://reviews.apache.org/r/26701/diff/3/?file=723865#file723865line2825> > > > > Could have the following above: > > > > // Must be set or unset together. > > CHECK(task->has_unacknoweldged_uuid() == > > task->has_unacknowledged_state()); done > On Oct. 18, 2014, 11:15 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 4549-4555 > > <https://reviews.apache.org/r/26701/diff/3/?file=723865#file723865line4549> > > > > 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..? done. > On Oct. 18, 2014, 11:15 p.m., Ben Mahler wrote: > > src/tests/master_tests.cpp, lines 2363-2366 > > <https://reviews.apache.org/r/26701/diff/3/?file=723866#file723866line2363> > > > > Quite the long sentence ;) rephrased. > On Oct. 18, 2014, 11:15 p.m., Ben Mahler wrote: > > src/tests/master_tests.cpp, line 2367 > > <https://reviews.apache.org/r/26701/diff/3/?file=723866#file723866line2367> > > > > We should probably encode the release of resources in the name? > > > > ReleaseResourcesForTerminalTaskWithPendingUpdates ok. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26701/#review57285 ----------------------------------------------------------- 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 > >
