----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26700/#review56651 -----------------------------------------------------------
I like the direction this is going. Our internal bandaid fix set the latest/terminal_state in stream->next instead of forward(), but I don't think it makes much of a difference. A few questions: - Where does the uuid come into play? - Why track latest_state for anything but the terminal state? src/messages/messages.proto <https://reviews.apache.org/r/26700/#comment97056> Kinda confusing that (at some point in time) StatusUpdate.status.state == Task.unacknowledged_state.state and Task.state == StatusUpdate.latest_state.state (did I get that right?) I guess it would be too hard to rename the existing protobufs and make schedulers/executors recompile. src/messages/messages.proto <https://reviews.apache.org/r/26700/#comment97043> Why does the StatusUpdate.latest_state need the other update's uuid? Why can't it just be a plain TaskState? I don't see uuid being used here. src/slave/status_update_manager.cpp <https://reviews.apache.org/r/26700/#comment97044> Also note that this method updates the stream's timeout? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/26700/#comment97046> s/atleast/at least/ src/slave/status_update_manager.cpp <https://reviews.apache.org/r/26700/#comment97047> Is there any reason to send this if it's not a terminal state? Otherwise, you're just diffing between RUNNING and RUNNING. src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26700/#comment97048> s/udpate/update/ src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26700/#comment97049> Times(1) is implicit, unnecessary. src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26700/#comment97050> Could use LaunchTasks() pattern instead of saving the offer src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26700/#comment97051> Verify state is TASK_RUNNING? src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26700/#comment97052> s/FINISHED/RUNNING/ src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26700/#comment97054> Verify the uuid value too? - Adam B On Oct. 14, 2014, 11:04 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26700/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2014, 11:04 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 > ------- > > Status update manager now sends both latest and unacknowledged states to the > master. > > > Diffs > ----- > > src/messages/messages.proto 8de7f9699f43aa2780f4a39fed087abcf5e5af99 > src/slave/status_update_manager.cpp > 5d5cf234ef2dd47fa4b1f67be761dbca31659451 > src/tests/status_update_manager_tests.cpp > e9ef1e208cb01535e9366db7872b922c8f06ec40 > > Diff: https://reviews.apache.org/r/26700/diff/ > > > Testing > ------- > > make check > > Ran new test 1000 times. > > > Thanks, > > Vinod Kone > >
