> On Oct. 15, 2014, 4:52 a.m., Adam B wrote: > > 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?
* uuid comes into play in the next review * we use latest_state for consistency with what the slave reports. makes the state updating logic in master simpler (see next review). > On Oct. 15, 2014, 4:52 a.m., Adam B wrote: > > src/messages/messages.proto, lines 80-83 > > <https://reviews.apache.org/r/26700/diff/1/?file=720973#file720973line80> > > > > 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. You got that right. And yes, changing the old names would be upgradability/compatibility nightmare. Suggestions for better names for the new fields I added are welcome. > On Oct. 15, 2014, 4:52 a.m., Adam B wrote: > > src/messages/messages.proto, line 83 > > <https://reviews.apache.org/r/26700/diff/1/?file=720973#file720973line83> > > > > 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. You are right! This could just be TaskState. Fixed. Thanks. > On Oct. 15, 2014, 4:52 a.m., Adam B wrote: > > src/slave/status_update_manager.cpp, lines 415-416 > > <https://reviews.apache.org/r/26700/diff/1/?file=720974#file720974line415> > > > > Is there any reason to send this if it's not a terminal state? > > Otherwise, you're just diffing between RUNNING and RUNNING. see above. done this mainly for consistency. > On Oct. 15, 2014, 4:52 a.m., Adam B wrote: > > src/tests/status_update_manager_tests.cpp, line 902 > > <https://reviews.apache.org/r/26700/diff/1/?file=720975#file720975line902> > > > > Verify state is TASK_RUNNING? see my comments in previous review. > On Oct. 15, 2014, 4:52 a.m., Adam B wrote: > > src/tests/status_update_manager_tests.cpp, line 935 > > <https://reviews.apache.org/r/26700/diff/1/?file=720975#file720975line935> > > > > Verify the uuid value too? N/A since we no longer include uuid. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26700/#review56651 ----------------------------------------------------------- On Oct. 14, 2014, 6:04 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26700/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2014, 6:04 p.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 > >
