> On Oct. 14, 2014, 10:51 p.m., Adam B wrote: > > src/messages/messages.proto, line 45 > > <https://reviews.apache.org/r/26698/diff/1/?file=720965#file720965line45> > > > > Maybe 'unacknowledged_status' would be more accurate, since the uuid is > > associated with the StatusUpdate message itself, and this could be one of > > many status updates with the TASK_RUNNING state, of which some may have > > already been acknowledged and others are waiting behind this one. In that > > case, the TASK_RUNNING state has already been received and acknowledged, > > but it's the other pending status updates that are unacknowledged. Maybe > > 'pending_state' or 'pending_status' would be better.
The hard part in naming here is because the Task.State.state will still be in the state after an ACK has been received by the master/slave. It only changes when the next update is received by the status update manager. Let me sleep on it. > On Oct. 14, 2014, 10:51 p.m., Adam B wrote: > > src/messages/messages.proto, line 58 > > <https://reviews.apache.org/r/26698/diff/1/?file=720965#file720965line58> > > > > This uuid corresponds to the uuid of a StatusUpdateMessage not the > > state itself. Why wouldn't you want to include the entire StatusUpdate or > > TaskStatus message instead of picking only the fields you think you need > > now? I'm sure the arbitrary size of the 'data' field comes into play, but > > I'd just like to know if you considered it and why you rejected it. > > > > Is uuid really always going to be required? Maybe it should be called > > 'status_uuid'? Yes. Preferrably I would have liked to just use StatusUpdate here instead of coming up with a new inner message. But after looking at recent reports about 'data' OOMing the master, I went for the conservative approach. Maybe we can just use StatusUpdate but scrub the un-necessary fields? Not sure which one is better and less confusing. As you must have seen in one of the subsequent reviews, master needs the uuid to correctly deal with status update acks. > On Oct. 14, 2014, 10:51 p.m., Adam B wrote: > > src/tests/status_update_manager_tests.cpp, line 807 > > <https://reviews.apache.org/r/26698/diff/1/?file=720968#file720968line807> > > > > Times(1) is implicit, unneeded. > > "If neither WillOnce() nor WillRepeatedly() is in the EXPECT_CALL(), > > the inferred cardinality is Times(1)." > > - > > https://code.google.com/p/googlemock/wiki/ForDummies#Cardinalities:_How_Many_Times_Will_It_Be_Called? > > (No offense) Yep. Overlooked it here and in other reviews, as i copy pasted stuff from other tests :) will fix. > On Oct. 14, 2014, 10:51 p.m., Adam B wrote: > > src/tests/status_update_manager_tests.cpp, line 806 > > <https://reviews.apache.org/r/26698/diff/1/?file=720968#file720968line806> > > > > Maybe you'd like to fill in some of these '_' blanks to at least > > guarantee the messages are coming from and going to the right pids? We typically don't explicitly set "_" unless we are interested in them. Makes for less cruft in the test. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26698/#review56583 ----------------------------------------------------------- On Oct. 14, 2014, 6 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26698/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2014, 6 p.m.) > > > Review request for mesos, Adam B, Ben Mahler, and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Added StatusUpdateManager::unacknowledged() API call so that slave can use > this info during reregistration. > > > Diffs > ----- > > src/messages/messages.proto 8de7f9699f43aa2780f4a39fed087abcf5e5af99 > src/slave/status_update_manager.hpp > 24e3882ad1969c20a64daf90e408618c310e9a81 > src/slave/status_update_manager.cpp > 5d5cf234ef2dd47fa4b1f67be761dbca31659451 > src/tests/status_update_manager_tests.cpp > e9ef1e208cb01535e9366db7872b922c8f06ec40 > > Diff: https://reviews.apache.org/r/26698/diff/ > > > Testing > ------- > > make check > > Ran new test 1000 times. > > > Thanks, > > Vinod Kone > >
