----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26698/#review56583 -----------------------------------------------------------
src/messages/messages.proto <https://reviews.apache.org/r/26698/#comment96929> 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. src/messages/messages.proto <https://reviews.apache.org/r/26698/#comment96928> 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'? src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96934> s/udpate/update/ src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96935> state and uuid? src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96940> 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? src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96939> 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) src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96941> Could just use the "LaunchTasks(DEFAULT_EXECUTOR_INFO, 1, 1, 64, "*")" pattern and avoid tracking the offers or directly calling driver.launchTasks. src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96938> Times(1) is implicit, unneeded. src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96942> s/unacknowledged/unacknowledged state/ src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/26698/#comment96943> Also validate the uuid? - Adam B On Oct. 14, 2014, 11 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26698/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2014, 11 a.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 > >
